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

Part 3: Multi-db Improvements, identifying replica configurations #33770

Merged
merged 3 commits into from Sep 1, 2018

Conversation

Projects
None yet
4 participants
@eileencodes
Member

eileencodes commented Aug 31, 2018

I'm going to start opening smaller PR's now that the refactoring is merged. This PR adds a few things:

  1. Adds a replica option to the configurations
  2. Adds a check on HashConfig and UrlConfig for replica? that checks for config["replica"] key
  3. Updates configs_for to take kwargs
  4. Updates configs_for to take a kwarg for include_replicas that defaults to false. When you're creating/dropping/migrating dbs you don't need to run the create/drop/migrate for the replicas as well. I defaulted to not including replicas because 99% of the time when you're iterating over the dbs (like creating the tasks, running the tasks etc) you don't actually want the replicas. This prevents messages like "database already exists" when running the create command.

cc/ @tenderlove @matthewd @rafaelfranca

eileencodes added some commits Aug 30, 2018

Add config option for `replica`.
This allows the user to add `replica: true` to the database config to
signify the connection should be treated as readonly. This will be
useful so we can ignore structure dumps or migrations (or creating /
deleting etc) the readonly connection for the databases. These are
paired with a write database which is where the create/drop/migrate
should be run. This allows us to ask the connection if it's for a
replica readonly db or a primary write db.
Add replica? check to DatabaseConfig
Checks if the config has a "replica" key, if so the configuration is for
a replica database. This is useful for excluding replicas from the
configurations list when creating the rake tasks or running rake tasks.
For example, we don't want to create the primary and primary_readonly.
They're the same database.

@eileencodes eileencodes added this to the 6.0.0 milestone Aug 31, 2018

@eileencodes eileencodes self-assigned this Aug 31, 2018

Convert configs_for to kwargs, add include_replicas
Changes the `configs_for` method from using traditional arguments to
using kwargs. This is so I can add the `include_replicas` kwarg without
having to always include `env_name` and `spec_name` in the method call.

`include_replicas` defaults to false because everywhere internally in
Rails we don't want replicas. `configs_for` is for iterating over
configurations to create / run rake tasks, so we really don't ever need
replicas in that case.
@@ -127,6 +127,10 @@ def initialize(connection, logger = nil, config = {}) # :nodoc:
)
end
def replica?
@config[:replica] || false

This comment has been minimized.

@tgxworld

tgxworld Sep 1, 2018

Contributor

@eileencodes Is it in the scope of this PR for each database adapter to verify if the replica database is indeed a replica to help catch user errors? We have this guard in place at Discourse whenever we switch over to a replica database:

  value = connection.exec_query("SELECT pg_is_in_recovery()").rows[0][0]
  raise "Replica database server is not in recovery mode." if !value
@tgxworld

tgxworld Sep 1, 2018

Contributor

@eileencodes Is it in the scope of this PR for each database adapter to verify if the replica database is indeed a replica to help catch user errors? We have this guard in place at Discourse whenever we switch over to a replica database:

  value = connection.exec_query("SELECT pg_is_in_recovery()").rows[0][0]
  raise "Replica database server is not in recovery mode." if !value

This comment has been minimized.

@eileencodes

eileencodes Sep 1, 2018

Member

At GitHub we have rw databases (primary) and ro databases (replica). The replicas have different users than the primary ones. In addition we also have readonly connections - a rw database can be connected to in a ro way. This change is for the first part - to be able to identify the replica database in the configurations list. Later when I implement the connection switching I'll handle your case as well since we need that at GitHub, but that's going to be a separate PR.

@eileencodes

eileencodes Sep 1, 2018

Member

At GitHub we have rw databases (primary) and ro databases (replica). The replicas have different users than the primary ones. In addition we also have readonly connections - a rw database can be connected to in a ro way. This change is for the first part - to be able to identify the replica database in the configurations list. Later when I implement the connection switching I'll handle your case as well since we need that at GitHub, but that's going to be a separate PR.

This comment has been minimized.

@tgxworld

tgxworld Sep 1, 2018

Contributor

That is great to hear :) Thank you for sharing.

@tgxworld

tgxworld Sep 1, 2018

Contributor

That is great to hear :) Thank you for sharing.

@eileencodes eileencodes merged commit d8ac08f into rails:master Sep 1, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eileencodes eileencodes deleted the eileencodes:multi-db-improvements-part-3 branch Sep 1, 2018

@mPanasiewicz

This comment has been minimized.

Show comment
Hide comment
@mPanasiewicz

mPanasiewicz commented Sep 2, 2018

@eileencodes Awesome!

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 8, 2018

Fix expanation of `ActiveRecord::Base.configurations.configs_for` in …
…the CHANGELOG

Since #33770 `#configs_for` changed method signature and it isn't
supposed to work with a passed block.

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 8, 2018

Fix explanation of `ActiveRecord::Base.configurations.configs_for` in…
… the CHANGELOG

Since #33770 `#configs_for` changed method signature and it isn't
supposed to work with a passed block.

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 9, 2018

Fix explanation of `ActiveRecord::Base.configurations.configs_for` in…
… the CHANGELOG

Since #33770 `#configs_for` changed method signature and it isn't
supposed to work with a passed block.

eileencodes added a commit that referenced this pull request Sep 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment