Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Proof of Concept] Make Semian work with activerecord-trilogy-adapter #435

Closed
wants to merge 24 commits into from

Conversation

adrianna-chang-shopify
Copy link
Contributor

@adrianna-chang-shopify adrianna-chang-shopify commented Oct 31, 2022

An alternative approach to #420, where we target resource acquisition at the Active Record adapter layer instead of patching the Trilogy client directly.

This is a simpler approach, and allows us to deal with native Active Record errors instead of the various errors Trilogy is currently raising. The tests are also a lot simpler because we don't have to handle re-instantiating the Trilogy client every time the connection is closed (either server or client side).

Caveats:

  • We'll need to deal with SFR separately
  • There is still work to be done in activerecord-trilogy-adapter to make some of the error classes better (specifically in #new_client)

@adrianna-chang-shopify adrianna-chang-shopify force-pushed the ac-trilogy-adapter-adapter branch 3 times, most recently from 7f0a908 to 3bc79fc Compare November 1, 2022 13:26
@adrianna-chang-shopify adrianna-chang-shopify force-pushed the ac-trilogy-adapter-adapter branch 2 times, most recently from 8010ef8 to 5c8ddc2 Compare November 1, 2022 19:13
Comment on lines 56 to 73
def with_resource_timeout(temp_timeout)
if connection.nil?
prev_read_timeout = @config[:read_timeout] || 0 # We're assuming defaulting the timeout to 0 won't change in Trilogy
@config[:read_timeout] = temp_timeout # Create new client with config
else
prev_read_timeout = connection.read_timeout
connection.read_timeout = temp_timeout
end
yield
ensure
@config[:read_timeout] = prev_read_timeout
connection&.read_timeout = prev_read_timeout
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so unfortunately making half_open_resource_timeout work is still a bit hacky, even working at the adapter layer. I'm racking my brain for another way to do this, but not sure we have many options.

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 can reset the timeout on the connection, and I'm assuming we should also reset the config.

This is additionally complex because if no read_timeout is set in the config, we need to use the client's default, which I know is 0 but could potentially change.

@adrianna-chang-shopify
Copy link
Contributor Author

Superseded by #468

@adrianna-chang-shopify adrianna-chang-shopify deleted the ac-trilogy-adapter-adapter branch February 16, 2023 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant