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

Removes manual establish_connection in interceptor #209

Merged

Conversation

bc-amit
Copy link
Contributor

@bc-amit bc-amit commented Apr 5, 2024

What?

Removes manual establish_connection from the interceptor. It seems like it was added here to support Rails 5.

Why?

  • This should occur automatically if/when during the request processing in the grpc service, ActiveRecord is used to perform a query.
  • For when sql server is unavailable. By checking connection.active? for each request and subsequently attempting to establish_connection, we are currently raising an exception (Can't connect to MySQL server etc.) in the interceptor before we can execute the Check inside health check endpoints. That seems incorrect.

How was it tested?

It has been tested against 3 rails versions [6.1, 7.0, 7.1] using gruf-demo repo. In all three tests, we have a gruf server running and rake task provided in the demo repo is used to exercise the GRPC service that executes sql queries.

I've recorded videos for testing against all 3 versions. We can see in the tests that sql query gets executed as expected and we are able to view results from the DB without manual connection establishment that comes from the interceptor.

  1. 6.1 - commit that includes current PR as dependency.
Rails-6-1-ruby-3.1.2-gruf.mov
  1. 7.0 - commit that upgrades Rails to version 7.0.
Rails-7-0-ruby-3.1.2-gruf.mov
  1. 7.1 - commit that upgrades Rails to version 7.1.
Rails-7-1-ruby-3.1.2-gruf.mov

@bc-amit bc-amit marked this pull request as ready for review April 5, 2024 06:18
@rabinshr
Copy link

rabinshr commented Apr 8, 2024

ping @bigcommerce/ruby

@TomA-R TomA-R requested a review from a team April 17, 2024 01:45
Copy link
Member

@splittingred splittingred left a comment

Choose a reason for hiding this comment

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

Thanks for doing the due diligence in testing here, @bc-amit . LGTM.

@splittingred splittingred merged commit e1c7822 into bigcommerce:main Apr 24, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants