Semian + Trilogy: Introduce adapter for Trilogy AR Adapter

[adrianna-chang-shopify]

We are planning to migrate Shopify/shopify (and our other Rails applications) to the Trilogy client instead of Mysql2. This PR introduces a Semian adapter for the Trilogy Active Record adapter.

Why isn't this built at the Trilogy client layer?

Building the Semian adapter at the Trilogy lient layer is challenging because of the way Trilogy manages its connection to the server. Any time an error is encountered, Trilogy will close the connection, and automatic reconnects are not supported. This means that TRILOGY_CONNECTION_CLOSED errors can happen for various reasons: if the network is down, but also if some other error was encountered and the connection is no longer usable. We’d need to make non-trivial changes to Trilogy to differentiate these use cases. Targeting the AR adapter layer allows us to lean on Rails to reconnect as needed, and allows us to differentiate between true connection errors (e.g. the network is down), vs Trilogy having closed the connection itself.

Why don't we write a Semian adapter that targets AbstractAdapter or AbstractMysqlAdapter?

I experimented with this here, but decided this wasn't a sensible option. Active Record translates certain errors we care about to ActiveRecord::StatementInvalid, and we need to look at the underlying error to see if it's something that should trigger the Semian circuit breaker (e.g. a timeout error). We'd need to bake in references to the various adapter classes (e.g. Trilogy::TimeoutError, Mysql2::Error), so this doesn't make sense.

---

I've tested this adapter out with Shopify/shopify and have a passing CI build: https://buildkite.com/shopify/shopify-selective-tests/builds/969555

Feedback

Dealing with half_open_resource_timeout is unfortunately still a bit hacky, even at the adapter layer. If anyone has thoughts on how to do this better, I'm all ears.

Basically, the Trilogy adapter forwards configs to the client, one of which is the read_timeout. In the context of with_resource_timeout, we might be connecting to the db, in which case connection is nil. Consequently, the only way I see being able to set a new (temp) read_timeout is to mutate the @config so that the newly instantiated client gets created with the timeout we want. If we have the connection already, we can reach directly to the connection (the Trilogy client) and modify the read_timeout.

In the ensure block, we restore the original timeout on the connection, and I'm assuming we should also reset the @config.

This is even more hacky because if no read_timeout is set in the config, we need to use the client's default (0), which I've hardcoded into the adapter, but could potentially change on Trilogy's end down the road...

[eileencodes]

I left one comment where I think we need to change the repo.

Thank you for the great explanation on the route you chose and why, makes a lot of sense. I don't love how mysql2 and trilogy are so different here but I understand that there's not a better option right now. I know we're working on reconnects so we should revisit this implementation later on.

[eileencodes]

This looks good to me but let's get @Shopify/byroot and the Semian owners to review before merging.

[paarthmadan]

Nice, the tests read really well 🙌

[adrianna-chang-shopify]

Pings in the Mysql2 client are circuit broken, primarily for the flow where we're establishing a connection and we call #verify! => #active? => #ping (more context in this PR). To mimic this at the adapter level, we should make sure #active? is circuit broken in the same way.

I've made the scope :ping to make it more similar to what the Semian Mysql2 adapter was doing, and since :ping is technically the method that talks to the db, but I could change this to :active if that makes more sense :)

[miry]

Hi,

I am going to release next version in next few days.
There are a plenty of changes related to performance optimization.

We can add a new adapter to the next version and release this week as well.