-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Merge ConnectionSpecification + Role -> Role #37503
Merge ConnectionSpecification + Role -> Role #37503
Conversation
In order to move schema_cache off of DatabaseConfiguration we needed to make role accessible on pool. While looking at `ConnectionSpecification`, `Role`, and `DatabaseConfig` John and I noticed that this could be achieved by merging `ConnectionSpecification` and `Role` into one `Role` class. This allows us to eliminate the `spec` concept which is confusing. `spec` is a private method so renaming to `resolve_role` is ok. In the `Role` class we took `name` (renamed to `connection_specification_name` for clarity since it's not a `role` name) and `db_config` from `ConnectionSpecification` and the `pool` methods from `Role` and combined them into one `Role` class. This feels a lot cleaner to us because it clarifies the purposes of the classes/methods/variables, and makes it easier to drop `connection_specification_name` keyed on the parent class in the future. There are a lot of changes in here but the majority of them are find and replace `spec` -> `role`, `spec.name` -> `role.connection_specification_name`, `Resolver#spec` -> `Resolver#resolve_role`. This PR also moves the `schema_cache` from `DatabaseConfig` to the new combined `Role` class. Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
I don't know if this is where you all at Shopify were planning on going with the code around ConnSpec and Role, but it started to feel like we had 2 classes doing kind of the same job. Happy to discuss more but wanted to open this PR as a discussion point and to see how the code feels when merging the 2 classes. This also unblocks #37368 which is failing locally but not on CI (I think because CI isn't running in isolation). Moving schema cache off DatabaseConfig resolves those local failures. |
@Edouard-chin could you also review this to see if this would break the PR you are working on. But I'm very happy with this direction. |
On mobile so only did a superficial review, but yes it looks a lot like where I was trying to get at. If there is no urgency I’ll do a full review Sunday/Monday |
ACK, I'll look if that doesn't conflict too much with the connection handling refactor we are doing with Jean. Will review later tonight or tomorrow ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea !
see if this would break the PR you are working on.
Nop it shouldn't conflict with what we are doing 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
@@ -0,0 +1,63 @@ | |||
# frozen_string_literal: true | |||
|
|||
module ActiveRecord |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to moving it to its own file. connection_pool.rb
is huge and painful to work with.
|
||
def initialize(connection_specification_name, db_config) | ||
super() | ||
@connection_specification_name = connection_specification_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is ok for now, but I'd really like to get rid of connection_specification_name
.
IMHO it's backwards. It's not the Role / DbConfig
that should have access to the root model identifier (AnimalsBase
), but the root model that should know what Role / DbConfig
it connects to (animals_replica
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely want to get rid of this but for now this seemed like a good compromise. Continue using it but name it something obvious that screams "get me out of here" 😅
|
||
module ActiveRecord | ||
module ConnectionAdapters | ||
class ConnectionSpecification # :nodoc: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
This was a typo from rails#37503 where we instantiate a `Role` twice. We already have a role so we don't need to create it again.
In order to move schema_cache off of DatabaseConfiguration we needed to
make role accessible on pool.
While looking at
ConnectionSpecification
,Role
, andDatabaseConfig
Johnand I noticed that this could be achieved by merging
ConnectionSpecification
and
Role
into oneRole
class. This allows us to eliminate thespec
concept which is confusing.
spec
is a private method so renamingto
resolve_role
is ok.In the
Role
class we tookname
(renamed toconnection_specification_name
for clarity since it's not a
role
name) anddb_config
fromConnectionSpecification
and thepool
methods fromRole
and combinedthem into one
Role
class.This feels a lot cleaner to us because it clarifies the purposes of the
classes/methods/variables, and makes it easier to drop
connection_specification_name
keyed on the parent class in the future.There are a lot of changes in here but the majority of them are find and
replace
spec
->role
,spec.name
->role.connection_specification_name
,Resolver#spec
->Resolver#resolve_role
.This PR also moves the
schema_cache
fromDatabaseConfig
to the newcombined
Role
class.Co-authored-by: John Crepezzi john.crepezzi@gmail.com
cc/ @seejohnrun @rafaelfranca @tenderlove @jhawthorn @casperisfine @etiennebarrie