Page MenuHomePhabricator

Consider phasing out ILoadBalancer::getLazyConnectionRef in favour of ILoadBalancer::getConnectionRef
Closed, ResolvedPublic

Description

Follow-up from T236880: Document when to use different ILoadBalancer::get*Connection* methods.

These two methods take the same arguments, and return the same logical object, and of the exact same type (DBConnRef).

The only difference:

  • getConnectionRef will pre-connect to the underlying database, thus the DBConnRef is only providing automatic re-use once out of scope.
  • getLazyConnectionRef lets DBConnRef auto-connect upon first relevant method call, and also provides the same automatic re-use once out of scope.

From what I can tell, deferring the connect call is not expensive, and the __call() overhead for auto-connecting is present in both cases as well.

Would it make sense to change getConnectionRef to also be lazy, and then phase out getLazyConnectionRef?

Is there a use case from a feature/product perspective of where code wants to preconnect to get a desirable change in behaviour or some other benefit?

Strawman proposal:

  • Make getConnectionRef do lazy connections by default. I'm not aware of cases where an eager connection is useful (whether for correctness or perf/stability etc). If such use case does come up that we missed (perhaps @aaron knows), then assuming they are rare, perhaps we can expose a "connect" method so that people can simply do getConnectionRef and then connect in order to get the eager connection they want before passing the connection around. Seems simple enough.
  • Deprecate and remove getLazyConnectionRef.
  • Make getConnection an alias for getConnectionRef, and turn reuseConnection into a deprecated no-op. Internally we'll move the code in reuseConnection to a different @internal-only method for DBConnRef to continue to use.
  • Remove reuseConnection.
  • Soft-deprecate getConnectionRef with no intention to remove until at least 2 LTS'es later. It's a commonly used method for a good reason, let's not have another major breaking churn here and keep it as a simple alias.

Details

ProjectBranchLines +/-Subject
mediawiki/coremaster+25 -35
mediawiki/coremaster+79 -89
mediawiki/extensions/Wikibasemaster+2 -2
mediawiki/coremaster+118 -121
mediawiki/extensions/ThrottleOverridemaster+1 -1
mediawiki/coremaster+15 -62
mediawiki/extensions/GrowthExperimentsmaster+8 -8
mediawiki/extensions/Wikibasemaster+2 -6
mediawiki/extensions/WikimediaEditorTasksmaster+4 -4
mediawiki/extensions/MediaSearchmaster+2 -2
mediawiki/extensions/ReadingListsmaster+1 -1
mediawiki/extensions/GlobalCssJsmaster+1 -1
mediawiki/extensions/VisualEditormaster+1 -1
mediawiki/extensions/MachineVisionmaster+13 -13
mediawiki/coremaster+2 -1
mediawiki/extensions/OAuthmaster+1 -1
mediawiki/extensions/Echomaster+2 -2
mediawiki/extensions/Translatemaster+1 -1
mediawiki/extensions/Babelmaster+1 -1
mediawiki/extensions/CheckUsermaster+2 -2
mediawiki/corewmf/1.38.0-wmf.23+1 -11
mediawiki/corewmf/1.38.0-wmf.24+1 -11
mediawiki/coremaster+1 -11
Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

We discussed in clinic duty triage, and we think this would make a good small project.

It is still useful to have a gauge of connectivity, in some cases, before attempting to use DB handles. That makes me lean towards keeping both out of convenience.

It is still useful to have a gauge of connectivity, in some cases, before attempting to use DB handles. That makes me lean towards keeping both out of convenience.

If the lazy behavior is preferred in most cases, it should be the default, and the implementation of the method that is already being called should be changed to be lazy. If we need non-lazy behavior in some cases, we could add a method for that - or just make it a flag to be passed in via $flags.

It is still useful to have a gauge of connectivity, in some cases, before attempting to use DB handles. That makes me lean towards keeping both out of convenience.

What is an example of where use of an eager connection is required or benefical for a specific feature and/or beneficial to the infra in which the feature is deployed?

I don't think our developers currently have the information neccecary from reading our code and docs to know what is right for them, or why.

I'm a big fan of this. This has been brought up as a confusing logic basically every time I ask developers what they find suboptimal in rdbms code.

Change 767469 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] [POC] Make getConnectionRef return with getLazyConnectionRef

https://gerrit.wikimedia.org/r/767469

The tests pass and generally looks fine. I really would like to push this because in my checks of common requests, I see a lot of connections get opened and closed immediately without making any query (except setting the configuration) and this is adding a lot of overhead to requests that should be quite fast otherwise.

I also want to revisit usages of getConnection. I think lots of devs mistakenly picked that instead of getConnectionRef.

Since we're at it, and assuming that get(Lazy)ConnectionRef is what you'd want to use almost always, would it make sense (perhaps as a future change) to have getConnection implement the functionality currently in getLazyConnectionRef, and rename the other methods accordingly?

I totally agree with you (the most common method of no class should be as long as getLazyConnectionRef) but I'm not sure how we can make such a massive change without breaking a lot of stuff. One way to do it is to rename getLazyConnectionRef a new method like "getDB()" and migrate usages from all three other methods to that.

Strawman proposal:

  1. Make getConnectionRef do lazy connections by default. I'm not aware of cases where an eager connection is useful (whether for correctness or perf/stability etc). If such use case does come up that we missed (perhaps @aaron knows), then assuming they are rare, perhaps we can expose a "connect" method so that people can simply do getConnectionRef and then connect in order to get the eager connection they want before passing the connection around. Seems simple enough.
  2. Deprecate and remove getLazyConnectionRef.
  3. Make getConnection an alias for getConnectionRef, and turn reuseConnection into a deprecated no-op. Internally we'll move the code in reuseConnection to a different @internal-only method for DBConnRef to continue to use.
  4. Remove reuseConnection.
  5. Soft-deprecate getConnectionRef with no intention to remove until at least 2 LTS'es later. It's a commonly used method for a good reason, let's not have another major breaking churn here and keep it as a simple alias.

Strawman proposal:

Sounds good to me.

The first step has a patch: https://gerrit.wikimedia.org/r/767469 ;)

I agree in general, if there is a need for edge cases in any way, we can have a method for that, the default should be simple.

Change 767469 merged by jenkins-bot:

[mediawiki/core@master] rdbms: Change getConnectionRef to return with getLazyConnectionRef

https://gerrit.wikimedia.org/r/767469

Change 767691 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@wmf/1.38.0-wmf.24] rdbms: Change getConnectionRef to return with getLazyConnectionRef

https://gerrit.wikimedia.org/r/767691

Change 767692 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@wmf/1.38.0-wmf.23] rdbms: Change getConnectionRef to return with getLazyConnectionRef

https://gerrit.wikimedia.org/r/767692

Change 767691 merged by jenkins-bot:

[mediawiki/core@wmf/1.38.0-wmf.24] rdbms: Change getConnectionRef to return with getLazyConnectionRef

https://gerrit.wikimedia.org/r/767691

Change 767692 merged by jenkins-bot:

[mediawiki/core@wmf/1.38.0-wmf.23] rdbms: Change getConnectionRef to return with getLazyConnectionRef

https://gerrit.wikimedia.org/r/767692

Mentioned in SAL (#wikimedia-operations) [2022-03-03T10:50:13Z] <ladsgroup@deploy1002> Synchronized php-1.38.0-wmf.24/includes/libs/rdbms/loadbalancer/LoadBalancer.php: Backport: [[gerrit:767691|rdbms: Change getConnectionRef to return with getLazyConnectionRef (T255493)]] (duration: 00m 51s)

Mentioned in SAL (#wikimedia-operations) [2022-03-03T10:58:32Z] <ladsgroup@deploy1002> Synchronized php-1.38.0-wmf.23/includes/libs/rdbms/loadbalancer/LoadBalancer.php: Backport: [[gerrit:767692|rdbms: Change getConnectionRef to return with getLazyConnectionRef (T255493)]] (duration: 00m 50s)

I backported the change, tested it thoroughly in production and nothing broke. After deployment also no new errors showed up, etc. Time to go to step 2!

Change 767760 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] rdbms: Deprecate getLazyConnectionRef

https://gerrit.wikimedia.org/r/767760

Change 767762 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/Translate@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/767762

Change 767763 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/OAuth@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/767763

Change 767764 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/Echo@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/767764

Change 767766 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/CheckUser@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/767766

Change 767768 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/Babel@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/767768

Change 767769 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] [POC] rdbms: Replace getConnection with getLazyConnectionRef

https://gerrit.wikimedia.org/r/767769

Change 767766 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/767766

Change 767768 merged by jenkins-bot:

[mediawiki/extensions/Babel@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/767768

Change 767762 merged by jenkins-bot:

[mediawiki/extensions/Translate@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/767762

Change 767764 merged by jenkins-bot:

[mediawiki/extensions/Echo@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/767764

Change 767763 merged by jenkins-bot:

[mediawiki/extensions/OAuth@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/767763

Change 767760 merged by jenkins-bot:

[mediawiki/core@master] rdbms: Deprecate getLazyConnectionRef

https://gerrit.wikimedia.org/r/767760

Change 768095 had a related patch set uploaded (by Gerrit maintenance bot; author: Gerrit maintenance bot):

[mediawiki/extensions/MachineVision@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/768095

Change 768096 had a related patch set uploaded (by Gerrit maintenance bot; author: Gerrit maintenance bot):

[mediawiki/extensions/GrowthExperiments@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/768096

Change 768097 had a related patch set uploaded (by Gerrit maintenance bot; author: Gerrit maintenance bot):

[mediawiki/extensions/GlobalCssJs@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/768097

Change 768098 had a related patch set uploaded (by Gerrit maintenance bot; author: Gerrit maintenance bot):

[mediawiki/extensions/MediaSearch@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/768098

Change 768099 had a related patch set uploaded (by Gerrit maintenance bot; author: Gerrit maintenance bot):

[mediawiki/extensions/ReadingLists@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/768099

Change 768100 had a related patch set uploaded (by Gerrit maintenance bot; author: Gerrit maintenance bot):

[mediawiki/extensions/VisualEditor@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/768100

Change 768101 had a related patch set uploaded (by Gerrit maintenance bot; author: Gerrit maintenance bot):

[mediawiki/extensions/WikimediaEditorTasks@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/768101

Change 768105 had a related patch set uploaded (by Gerrit maintenance bot; author: Gerrit maintenance bot):

[mediawiki/extensions/Wikibase@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/768105

Change 768095 merged by jenkins-bot:

[mediawiki/extensions/MachineVision@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/768095

Change 768100 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/768100

Change 768097 merged by jenkins-bot:

[mediawiki/extensions/GlobalCssJs@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/768097

Change 768098 merged by jenkins-bot:

[mediawiki/extensions/MediaSearch@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/768098

Change 768099 merged by jenkins-bot:

[mediawiki/extensions/ReadingLists@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/768099

Change 768101 merged by jenkins-bot:

[mediawiki/extensions/WikimediaEditorTasks@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/768101

Change 768105 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/768105

Change 768096 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Change use of deprecated getLazyConnectionRef

https://gerrit.wikimedia.org/r/768096

Change 769041 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] rdbms: Hard-deprecate LoadBalancer::getLazyConnectionRef

https://gerrit.wikimedia.org/r/769041

Change 769041 merged by jenkins-bot:

[mediawiki/core@master] rdbms: Hard-deprecate LoadBalancer::getLazyConnectionRef

https://gerrit.wikimedia.org/r/769041

I think what the title is implying has been achieved, I can make another ticket for phasing out getConnectionRef in favor of getConnection and pick it up right away. Does that sound good to you?

Change 771006 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/ThrottleOverride@master] Replace deprecated LoadBalancer::getLazyConnectionRef

https://gerrit.wikimedia.org/r/771006

Change 771006 merged by jenkins-bot:

[mediawiki/extensions/ThrottleOverride@master] Replace deprecated LoadBalancer::getLazyConnectionRef

https://gerrit.wikimedia.org/r/771006

Krinkle renamed this task from Consider phasing out ILoadBalancer::getConnectionRef in favour of ILoadBalancer::getLazyConnectionRef to Consider phasing out ILoadBalancer::getLazyConnectionRef in favour of ILoadBalancer::getConnectionRef.Mar 18 2022, 1:33 PM
Krinkle updated the task description. (Show Details)

Change 779165 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] rdbms: Fold MaintainableDBConnRef into DBConnRef

https://gerrit.wikimedia.org/r/779165

Change 779165 merged by jenkins-bot:

[mediawiki/core@master] rdbms: Fold MaintainableDBConnRef into DBConnRef

https://gerrit.wikimedia.org/r/779165

Change 781052 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/Wikibase@master] TermStore: Fix brtitle test

https://gerrit.wikimedia.org/r/781052

Change 781052 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] TermStore: Fix brittle test

https://gerrit.wikimedia.org/r/781052

Change 767769 merged by jenkins-bot:

[mediawiki/core@master] rdbms: Replace getConnection with getLazyConnectionRef

https://gerrit.wikimedia.org/r/767769

Ladsgroup moved this task from In progress to Done on the DBA board.

This is done.

Change 824236 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] rdbms: Update outdated docs around ILoadBalancer::getConn methods

https://gerrit.wikimedia.org/r/824236