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

Support multiple databases connection reset #180

Merged
merged 2 commits into from
Jan 15, 2023

Conversation

heka1024
Copy link
Contributor

@heka1024 heka1024 commented Dec 30, 2022

What? Why?

  • This PR support multiple databases connection reset
  • In our company, we use multiple databases like below.
production:
  primary:
    database: my_primary_database
    username: root
    password: <%= ENV['ROOT_PASSWORD'] %>
    adapter: mysql2
  animals:
    database: my_animals_database
    username: animals_root
    password: <%= ENV['ANIMALS_ROOT_PASSWORD'] %>
    adapter: mysql2
    migrations_paths: db/animals_migrate
class AnimalsRecord < ApplicationRecord
  self.abstract_class = true

  connects_to database: { writing: :animals }
end

(Copied from https://guides.rubyonrails.org/active_record_multiple_databases.html)

We use basic Gruf::Interceptors::ActiveRecord::ConnectionReset but there was connection pool error. After some debug, we discovered that it was a problem about our non-primary database, AnimalsRecord in above example.

I think it's better to support this feature in upstream. So, I make a PR for it. 😄

How was it tested?

  • My company uses it's hard-coded version (which replace target_classes to our ApplicationRecord's subclasses) in our production deployment.
  • I made some test cases.

Thanks for this Gem. I love it ❤️

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.

This is great, thanks @heka1024 !

Let's get #181 merged in, then you can rebase, and we can merge from there.

@heka1024 heka1024 force-pushed the multiple-databases branch from e4ae435 to 051e432 Compare January 7, 2023 17:29
@heka1024
Copy link
Contributor Author

heka1024 commented Jan 7, 2023

Thanks, @splittingred 😄 I rebase it with upstream!

@splittingred
Copy link
Member

@heka1024 Great! Can you update CHANGELOG.md with a short description of the changes? Thanks!

@splittingred splittingred merged commit 4a96b53 into bigcommerce:main Jan 15, 2023
@heka1024 heka1024 deleted the multiple-databases branch January 15, 2023 15:54
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.

2 participants