Simplify adapter construction; defer connect until first use #44591
Conversation
matthewd
force-pushed
the
defer-db-connect
branch
4 times, most recently
from
07d39b2
to
f534af2
Compare
matthewd
force-pushed
the
defer-db-verify
branch
2 times, most recently
from
4a45007
to
e02e06e
Compare
matthewd
force-pushed
the
defer-db-verify
branch
2 times, most recently
from
910340d
to
8ff11be
Compare
| @logger = ActiveRecord::Base.logger | ||
|
|
||
| if deprecated_logger || deprecated_connection_options || deprecated_config | ||
| raise ArgumentError, "new API accepts just one config hash" |
There was a problem hiding this comment.
This is a confusing error because if I passed a config hash and a logger I wouldn't know what this error was indicating I should change. I know this is a WIP but wanted to note I think each deprecated option should have it's own error to be clearer about the new API.
| if config_or_deprecated_connection.is_a?(Hash) | ||
| @raw_connection = nil | ||
| @config = config_or_deprecated_connection.symbolize_keys | ||
| @logger = ActiveRecord::Base.logger |
There was a problem hiding this comment.
Are we saying adapters can't set their own logger now? Do any adapters set their own logger? I think that'd be odd but not impossible.
There was a problem hiding this comment.
My plan is to ultimately pull the connection's base model from the handler (or pass it in the supplied config hash?), and then use that object's logger -- so you can't directly override the adapter's logger during initialize (which hardly anyone calls anyway -- note that mysql2_connection and friends are already just passing logger), but there is some control beyond the hard-coded AR::Base.
| nil, | ||
| config, | ||
| ) | ||
| ConnectionAdapters::Mysql2Adapter.new(config) |
| end | ||
|
|
||
| def reconnect!(restore_transactions: false) | ||
| @lock.synchronize do | ||
| @raw_connection.close | ||
| @raw_connection&.close |
There was a problem hiding this comment.
I don't like that now we have to keep checking for nil everywhere. I'm concerned we'll forget to do this in the future for a new method.
There was a problem hiding this comment.
I think it's fairly fine... we have plenty of potentially-nil ivars in various places. The main mitigating factor here is that we should rarely add new methods that use the variable directly: outside of connection management (dis/re-/connect), nearly everything else should use with_raw_connection, valid_raw_connection, or any_raw_connection (in that preference order).
The other side is just that there isn't much of an alternative: while abstract adapter defines that @raw_connection exists, is nil when not connected, and truthy when connected... the rest of the API is adapter-specific, so it's impossible for us to use a null-object pattern, say.
| @connection.disconnect! | ||
|
|
||
| assert_raise(ActiveRecord::ConnectionNotEstablished) do | ||
| @connection.quote("string") | ||
| end |
There was a problem hiding this comment.
Could this potentially be an undesirable change in behavior for applications?
There was a problem hiding this comment.
Theoretically, yes -- I believe this is the one observable difference in existing API behaviour, outside of timing.
I did consider separately tracking a concept of explicit disconnection, and having that persist until an equally-explicit reconnect... but I just can't come up with a reasonable use case we're breaking to justify the extra code / state. The obvious use for disconnect! would be that you e.g. want to restart the DB server, so need to break your existing session... but we still do that; we just reconnect on demand instead of leaving the adapter in a semi-permanent state of brokenness.
matthewd
force-pushed
the
defer-db-verify
branch
from
8ff11be
to
1c7f5dc
Compare
matthewd
force-pushed
the
defer-db-connect
branch
from
f534af2
to
feed930
Compare
matthewd
force-pushed
the
defer-db-connect
branch
2 times, most recently
from
4d641a4
to
cb87499
Compare
matthewd
force-pushed
the
defer-db-connect
branch
from
0e0e821
to
74adf63
Compare
matthewd
force-pushed
the
defer-db-verify
branch
from
e09e1f7
to
5012adc
Compare
matthewd
force-pushed
the
defer-db-connect
branch
from
74adf63
to
026626c
Compare
matthewd
force-pushed
the
defer-db-verify
branch
from
5012adc
to
7efb54e
Compare
matthewd
force-pushed
the
defer-db-connect
branch
from
026626c
to
e7a7b26
Compare
matthewd
force-pushed
the
defer-db-verify
branch
from
7efb54e
to
bc93b3a
Compare
matthewd
force-pushed
the
defer-db-connect
branch
2 times, most recently
from
8316373
to
74b99cf
Compare
Building on the work done in rails#44576 and rails#44591, we extend the logic that automatically reconnects broken db connections to take into account a timeout limit. This ensures that retries + reconnects are slow-query aware, and that we don't retry queries if a given amount of time has already passed since the query was first tried. This value will default to 5 seconds, but can be adjusted via the `connection_retry_timeout` config.
Building on the work done in rails#44576 and rails#44591, we extend the logic that automatically reconnects broken db connections to take into account a timeout limit. This ensures that retries + reconnects are slow-query aware, and that we don't retry queries if a given amount of time has already passed since the query was first tried. This value will default to 5 seconds, but can be adjusted via the `connection_retry_timeout` config.
Actually, it fails since this commit is merged. rails/rails@deec300
Make sure unit tests run successfully before rails/rails#44591 is merged
We can't call @raw_connection right away anymore, because it is nil until it is used after the changes in rails/rails#44576 and rails/rails#44591. Also we need to have @lock established in order to be able to use `with_raw_connection`. In the abstract_adapter implementation, that happens after the arel_visitor is set, but in oracle enhanced, we need to use `with_raw_connection` to check the database version in order to pick with visitor to use. Calling lock_thread = nil before super means @lock isi available to set the arel_visitor. Rails rails/rails#48188 also requires an internal_exec_query method that is called by the abstract adpater's exec_query. Naively renaming the oracle-enhanced's exec_query to internal_exec_query makes the oracle enhanced tests run. I also saw errors due to `active?` using @raw_connection. `valid_raw_connection` ends up calling `active?` via `with_raw_connection` and `verify` so that can't be used as a replacement for @raw_connection here. Borrowing from the postgres adapter implementation to get around this.
We can't call @raw_connection right away anymore, because it is nil until it is used after the changes in rails/rails#44576 and rails/rails#44591. Also we need to have @lock established in order to be able to use `with_raw_connection`. In the abstract_adapter implementation, that happens after the arel_visitor is set, but in oracle enhanced, we need to use `with_raw_connection` to check the database version in order to pick with visitor to use. Calling lock_thread = nil before super means @lock isi available to set the arel_visitor. Rails rails/rails#48229 also requires an internal_exec_query method that is called by the abstract adpater's exec_query. Naively renaming the oracle-enhanced's exec_query to internal_exec_query makes the oracle enhanced tests run. I also saw errors due to `active?` using @raw_connection. `valid_raw_connection` ends up calling `active?` via `with_raw_connection` and `verify` so that can't be used as a replacement for @raw_connection here. Borrowing from the postgres adapter implementation to get around this.
Building on from #44576, we no longer need to connect to the database in order to construct an adapter instance: we instead supply just the config hash, and it will connect on demand, in the same way it already knows to handle later reconnections.
This cleans up a weird API overlap, where we previously needed to know how to connect in two different places.