wfWaitForSlaves has been deprecated since 1.27 with a suggestion to use LBFactory::waitForReplication instead.
Usages in MediaWiki maintenance scripts and extensions should be replaced
There's about 160 odd usages in WMF extensions and the maintenance folder of MW core (code search)
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | None | T254646 Reconsidering how we name things | |||
| Resolved | Reedy | T181673 wfWaitForSlaves is deprecated, replace usages |
Note that the two have different default behavior that will need to be taken into account.
Most of the usages (without parameters passed, at least) just seem to have been swapped 1:1
wfWaitForSlaves(); -> wfGetLBFactory()->waitForReplication();
Which is my point. A call to wfWaitForSlaves() with no parameters looks to be equivalent to
wfGetLBFactory()->waitForReplication( [
'domain' => wfWikiID(),
'timeout' => PHP_SAPI === 'cli' ? 86400 : 10,
] )A call to wfGetLBFactory()->waitForReplication() with no parameters looks to be equivalent to wfWaitForSlaves( null, false, '*', 60 ).
Do we care about the different timeout (particularly in CLI mode) and the change from waiting for the local wiki to waiting for every cluster? We should decide that, not accidentally change it.
Additionally, wfWaitForSlaves() without a check on the return value will simply continue even if the wait failed. wfGetLBFactory()->waitForReplication() will throw an exception which will probably be uncaught by the calling code. Is that change, too, a good thing? We should decide, not accidentally make the change.
Hmm, if I understood Reedy correctly, some usages of wfWaitForSlaves() were already be converted to wfGetLBFactory()->waitForReplication() without keeping the notes from @Anomie in mind? What would be the best way to proceed here? Having the timeout reduced because of this change looks like a massive problem, as well as not waiting for the wiki but for every cluster.
More of the usages in /includes don't seem to care about the timeout differences...
Targets
Occurrences of 'waitForReplication' in Directory /Users/reedy/PhpstormProjects/mediawiki/core/includes
Found Occurrences (41 usages found)
Unclassified occurrence (35 usages found)
mediawiki (35 usages found)
core/includes (1 usage found)
GlobalFunctions.php (1 usage found)
3049 wfGetLBFactory()->waitForReplication( [
core/includes/deferred (8 usages found)
LinksDeletionUpdate.php (3 usages found)
93 $lbFactory->commitAndWaitForReplication(
189 $lbFactory->commitAndWaitForReplication(
211 $lbFactory->commitAndWaitForReplication(
LinksUpdate.php (5 usages found)
386 $lbf->commitAndWaitForReplication( __METHOD__, $this->ticket, [ 'domain' => $domainId ] );
390 $lbf->commitAndWaitForReplication(
396 $lbf->commitAndWaitForReplication(
472 $lbf->commitAndWaitForReplication(
480 $lbf->commitAndWaitForReplication(
core/includes/installer (2 usages found)
DatabaseUpdater.php (1 usage found)
486 $lbFactory->waitForReplication();
MysqlUpdater.php (1 usage found)
877 $lbFactory->waitForReplication( [ 'wiki' => wfWikiID() ] );
core/includes/jobqueue (2 usages found)
JobQueueDB.php (1 usage found)
547 $lbFactory->waitForReplication( [ 'wiki' => $this->wiki, 'cluster' => $this->cluster ] );
JobRunner.php (1 usage found)
229 $lbFactory->waitForReplication( [
core/includes/jobqueue/jobs (8 usages found)
CategoryMembershipChangeJob.php (2 usages found)
192 $lbFactory->commitAndWaitForReplication( __METHOD__, $this->ticket );
200 $lbFactory->commitAndWaitForReplication( __METHOD__, $this->ticket );
HTMLCacheUpdateJob.php (1 usage found)
141 $factory->commitAndWaitForReplication( __METHOD__, $ticket );
RecentChangesUpdateJob.php (2 usages found)
109 $factory->commitAndWaitForReplication(
217 $factory->commitAndWaitForReplication( __METHOD__, $ticket );
RefreshLinksJob.php (3 usages found)
94 $lbFactory->waitForReplication( [
258 $lbFactory->commitAndWaitForReplication( __METHOD__, $ticket );
290 $lbFactory->commitAndWaitForReplication( __METHOD__, $ticket );
core/includes/jobqueue/utils (1 usage found)
PurgeJobUtils.php (1 usage found)
75 $lbFactory->commitAndWaitForReplication( __METHOD__, $ticket );
core/includes/libs/rdbms/lbfactory (7 usages found)
ILBFactory.php (3 usages found)
244 public function waitForReplication( array $opts = [] );
254 public function setWaitForReplicationListener( $name, callable $callback = null );
274 public function commitAndWaitForReplication( $fname, $ticket, array $opts = [] );
LBFactory.php (4 usages found)
307 public function waitForReplication( array $opts = [] ) {
377 public function setWaitForReplicationListener( $name, callable $callback = null ) {
396 public function commitAndWaitForReplication( $fname, $ticket, array $opts = [] ) {
414 $this->waitForReplication( $opts );
core/includes/objectcache (3 usages found)
SqlBagOStuff.php (3 usages found)
378 $ok = $this->waitForReplication() && $ok;
490 $ok = $this->waitForReplication() && $ok;
798 protected function waitForReplication() {
core/includes/user (1 usage found)
User.php (1 usage found)
3813 $lbFactory->commitAndWaitForReplication( __METHOD__, $ticket );
core/includes/utils (1 usage found)
BatchRowWriter.php (1 usage found)
72 $lbFactory->commitAndWaitForReplication( __METHOD__, $ticket );
core/includes/watcheditem (1 usage found)
WatchedItemStore.php (1 usage found)
710 $factory->commitAndWaitForReplication(
Usage in comments (6 usages found)
mediawiki (6 usages found)
core/includes (1 usage found)
GlobalFunctions.php (1 usage found)
3032 * @deprecated since 1.27 Use LBFactory::waitForReplication
core/includes/deferred (1 usage found)
DeferredUpdates.php (1 usage found)
40 * - a) Any waitForReplication() call if no writes are pending on any DB
core/includes/libs/rdbms/lbfactory (4 usages found)
ILBFactory.php (4 usages found)
247 * Add a callback to be run in every call to waitForReplication() before waiting
260 * @return mixed A value to pass to commitAndWaitForReplication()
265 * Convenience method for safely running commitMasterChanges()/waitForReplication()
271 * @param array $opts Options to waitForReplication()Let me get this, LBFactory::waitForReplication is like a cron job? Running in the background when a DB update is needed for let's say maintenance? I've haven't used mediawiki for a while (since 1.25~), can I know more about these clusters?
No, nothing like a cron job. It's run in various places when we want to wait for slave databases to catch up with the master, for varying reasons. In maintenance scripts, this is so we know we're not causing any replication lag, and we can pull the most up to date information from the database
Change 394753 had a related patch set uploaded (by Lengyue; owner: Lengyue):
[mediawiki/core@master] core:Replace usage of wfWaitForSlaves.
tbh, I don't really get what's wrong with having convenience functions like wfWaitForSlaves(). Its not like we're reducing any global state by replacing it with an indirect global function call and it seems to supply some sane defaults.
Okay, I did some basic research on that topic. This is the place where wfWaitForSlaves() was deprecated: https://github.com/wikimedia/mediawiki/commit/fedfee628c377eeea0453ed82af02b6878bd525b
I think that for Backward Compatibility, the logic behind wfWaitForSlaves() stuff should stay unchanged because there is no need to change it anyways IMHO, but I see that this global function isn't a good solution to the problem, so I made a patch that fixes all that deprecations and at the same time saves the compatibility of behaviour of this method. I introduced new method, LBFactory->waitForSlaves() which has the same body as the wfWaitForSlaves() but uses the object oriented interface and because of that isn't transparent as the global function is.
I've also replaced usages of wfWaitForSlaves() with that function so that no global funciton is used to get it done.
What do you think?
Here's the patch: https://gerrit.wikimedia.org/r/#/c/395148/
Yes, this method is almost entirely (ok, not "almost", it's "entirely") copied from the deprecated one, but once the deprecated function will be removed, there will be no repetition whatsoever and I think that deprecated things shouldn't be counted in terms of DRY in this case.
Change 395148 had a related patch set uploaded (by Albert221; owner: Albert221):
[mediawiki/core@master] Replace wfWaitForSlaves() with not deprecated method
Change 394537 had a related patch set uploaded (by Zoranzoki21; owner: Haikalizz):
[mediawiki/core@master] core: Replace usage of wfWaitForSlaves
Change 395148 abandoned by Krinkle:
Replace wfWaitForSlaves() with not deprecated method
Change 536579 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] Stop using deprecated function wfWaitForSlaves()
Also note that the default behavior of both has changed since 2017. As far as I can tell, a wfWaitForSlaves() call without parameters is now equivalent to:
$lbFactory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory(); // TODO inject $lbFactory->waitForReplication( [ 'domain' => $lbFactory->getLocalDomainID(), ] );
Ia820196cac changed the default timeout (it’s now the same for both) and also made waitForReplication() no longer throw an exception, so I think the default domain/cluster is the only remaining difference.
Change 536579 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Stop using deprecated function wfWaitForSlaves()
Change 602832 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/AntiSpoof@master] Replace usage of wfWaitForSlaves()
Change 602833 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/CentralAuth@master] Replace usage of wfWaitForSlaves()
Change 602834 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/WikimediaMaintenance@master] Replace usage of CentalAuthUser::waitForSlaves()
Change 602837 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/CodeReview@master] Replace usage of wfWaitForSlaves()
Change 602838 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/CheckUser@master] Replace usage of wfWaitForSlaves()
Change 602839 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/ContentTranslation@master] Replace usage of wfWaitForSlaves()
Change 602840 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/CreditsSource@master] Replace usage of wfWaitForSlaves()
Change 602841 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/GeoData@master] Replace usage of wfWaitForSlaves()
Change 602842 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/GlobalBlocking@master] Replace usage of wfWaitForSlaves()
Change 602843 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/Newsletter@master] Replace usage of wfWaitForSlaves()
Change 602844 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/PageAssessments@master] Replace usage of wfWaitForSlaves()
Change 602845 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/ProofreadPage@master] Replace usage of wfWaitForSlaves()
I think the slight difference would be bearable in most cases. I'm not sure in all cases though.
Change 602837 merged by jenkins-bot:
[mediawiki/extensions/CodeReview@master] Replace usage of wfWaitForSlaves()
Change 602838 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@master] Replace usage of wfWaitForSlaves()
Change 602840 merged by jenkins-bot:
[mediawiki/extensions/CreditsSource@master] Replace usage of wfWaitForSlaves()
Change 602844 merged by jenkins-bot:
[mediawiki/extensions/PageAssessments@master] Replace usage of wfWaitForSlaves()
Change 602832 merged by jenkins-bot:
[mediawiki/extensions/AntiSpoof@master] Replace usage of wfWaitForSlaves()
Change 602839 merged by jenkins-bot:
[mediawiki/extensions/ContentTranslation@master] Replace usage of wfWaitForSlaves()
Change 602833 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@master] Replace usage of wfWaitForSlaves()
Change 602841 merged by jenkins-bot:
[mediawiki/extensions/GeoData@master] Replace usage of wfWaitForSlaves()
Change 602843 merged by jenkins-bot:
[mediawiki/extensions/Newsletter@master] Replace usage of wfWaitForSlaves()
Change 602842 merged by jenkins-bot:
[mediawiki/extensions/GlobalBlocking@master] Replace usage of wfWaitForSlaves()
Change 602834 merged by jenkins-bot:
[mediawiki/extensions/WikimediaMaintenance@master] Replace usage of CentalAuthUser::waitForSlaves()
Change 602853 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/WikimediaMaintenance@master] Replace usage of wfWaitForSlaves()
Change 602854 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/VisualEditor@master] Replace usage of wfWaitForSlaves()
Change 602855 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/UniversalLanguageSelector@master] Replace usage of wfWaitForSlaves()
Change 602856 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/Translate@master] Replace usage of wfWaitForSlaves()
Change 602857 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/ShortUrl@master] Replace usage of wfWaitForSlaves()
Change 602858 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/SecurePoll@master] Replace usage of wfWaitForSlaves()
Change 602860 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/TimedMediaHandler@master] Replace usage of wfWaitForSlaves()
Change 602854 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Replace usage of wfWaitForSlaves()
Change 602853 merged by jenkins-bot:
[mediawiki/extensions/WikimediaMaintenance@master] Replace usage of wfWaitForSlaves()
Change 602857 merged by jenkins-bot:
[mediawiki/extensions/ShortUrl@master] Replace usage of wfWaitForSlaves()
Change 602861 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/core@master] Hard deprecate wfWaitForSlaves()
Change 602862 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/OAuth@master] Replace usage of wfWaitForSlaves()
Change 602860 merged by jenkins-bot:
[mediawiki/extensions/TimedMediaHandler@master] Replace usage of wfWaitForSlaves()
Change 602864 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/PageTriage@master] Replace usage of wfWaitForSlaves()
Change 602866 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/FlaggedRevs@master] Replace usage of wfWaitForSlaves()
Change 602855 merged by jenkins-bot:
[mediawiki/extensions/UniversalLanguageSelector@master] Replace usage of wfWaitForSlaves()
Change 602858 merged by jenkins-bot:
[mediawiki/extensions/SecurePoll@master] Replace usage of wfWaitForSlaves()
Change 602856 merged by jenkins-bot:
[mediawiki/extensions/Translate@master] Replace usage of wfWaitForSlaves()
Change 602864 merged by jenkins-bot:
[mediawiki/extensions/PageTriage@master] Replace usage of wfWaitForSlaves()
Change 602867 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/Flow@master] Replace usage of wfWaitForSlaves()
Change 602862 merged by jenkins-bot:
[mediawiki/extensions/OAuth@master] Replace usage of wfWaitForSlaves()
Change 602866 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@master] Replace usage of wfWaitForSlaves()
Change 602868 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/Renameuser@master] Replace usage of wfWaitForSlaves()
Change 602869 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/TitleKey@master] Replace usage of wfWaitForSlaves()
Change 602870 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/DisableAccount@master] Replace usage of wfWaitForSlaves()
Change 602871 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/DumpHTML@master] Replace usage of wfWaitForSlaves()
Change 602872 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/CollapsibleVector@master] Replace usage of wfWaitForSlaves()
Change 602873 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/BlueSpiceEchoConnector@master] Replace usage of wfWaitForSlaves()
Change 602869 merged by jenkins-bot:
[mediawiki/extensions/TitleKey@master] Replace usage of wfWaitForSlaves()
Change 602868 merged by jenkins-bot:
[mediawiki/extensions/Renameuser@master] Replace usage of wfWaitForSlaves()
Change 602871 merged by jenkins-bot:
[mediawiki/extensions/DumpHTML@master] Replace usage of wfWaitForSlaves()
Change 602870 merged by jenkins-bot:
[mediawiki/extensions/DisableAccount@master] Replace usage of wfWaitForSlaves()
Change 602872 merged by jenkins-bot:
[mediawiki/extensions/CollapsibleVector@master] Replace usage of wfWaitForSlaves()
Change 602873 merged by jenkins-bot:
[mediawiki/extensions/BlueSpiceEchoConnector@master] Replace usage of wfWaitForSlaves()
Change 602867 merged by jenkins-bot:
[mediawiki/extensions/Flow@master] Replace usage of wfWaitForSlaves()
Change 602845 merged by jenkins-bot:
[mediawiki/extensions/ProofreadPage@master] Replace usage of wfWaitForSlaves()
Change 394753 abandoned by Ladsgroup:
core: Replace usage of wfWaitForSlaves
Reason:
This is done in another patch
Change 394537 abandoned by Ladsgroup:
core: Replace usage of wfWaitForSlaves
Reason:
This is done in another patch
Change 602861 merged by jenkins-bot:
[mediawiki/core@master] Hard deprecate wfWaitForSlaves()