From fbf4fe4357db5c08b1d8a46a7eeabdf306deaf09 Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Fri, 29 May 2020 10:34:45 +1000 Subject: [PATCH] In ChronologyProtector fix confusion between DB name and master name The one caller of LBFactory::getChronologyProtectorTouched() was calling it with a domain ID instead of a master server name. Using the master server name to identify replication position makes sense for ChronologyProtector, since the replication position may be reset when the master changes, but it is an odd convention for LBFactory. So: * Rename all $dbName variables in ChronologyProtector to $masterName, for clarity. * Interpret the first parameter to ILBFactory::getChronologyProtectorTouched() as a database domain, to make its only existing caller work. * Change the first parameter to ChronologyProtector::getTouched() from a string to a strongly typed ILoadBalancer, by analogy with applySessionReplicationPosition(). This removes the master name concept from the public interface. * Mark ChronologyProtector @internal. The accessor in LBFactory is protected, so extensions can't use it anyway. This is just to clarify why I think changing the parameter to getTouched() without b/c is OK. * Add a simple test which mostly just checks that ChronologyProtector gets called with the correct parameters. It's an LBFactory/ChronologyProtector integration test. Change-Id: I3b4832b5a4d7410e94b9c51577b30b31d49bc63d --- RELEASE-NOTES-1.35 | 3 +++ includes/libs/rdbms/ChronologyProtector.php | 18 +++++++++++------- includes/libs/rdbms/lbfactory/ILBFactory.php | 4 ++-- includes/libs/rdbms/lbfactory/LBFactory.php | 4 ++-- tests/phpunit/includes/db/LBFactoryTest.php | 17 +++++++++++++++++ 5 files changed, 35 insertions(+), 11 deletions(-) diff --git a/RELEASE-NOTES-1.35 b/RELEASE-NOTES-1.35 index 280e6de04c0..1de7d58963a 100644 --- a/RELEASE-NOTES-1.35 +++ b/RELEASE-NOTES-1.35 @@ -644,6 +644,9 @@ because of Phabricator reports. - AuthenticationProvider - ResourceLoaderModule - SearchEngine +* The parameters to ChronologyProtector::getTouched() and + ILBFactory::getChronologyProtectorTouched() were changed without backwards + compatibility. * … === Deprecations in 1.35 === diff --git a/includes/libs/rdbms/ChronologyProtector.php b/includes/libs/rdbms/ChronologyProtector.php index 35c12ea61ba..d85804c6e15 100644 --- a/includes/libs/rdbms/ChronologyProtector.php +++ b/includes/libs/rdbms/ChronologyProtector.php @@ -35,6 +35,8 @@ use Wikimedia\WaitConditionLoop; * This helps to ensure a consistent ordering of events as seen by an client * * Kind of like Hawking's [[Chronology Protection Agency]]. + * + * @internal */ class ChronologyProtector implements LoggerAwareInterface { /** @var BagOStuff */ @@ -269,21 +271,23 @@ class ChronologyProtector implements LoggerAwareInterface { } /** - * @param string $dbName DB master name (e.g. "db1052") + * @param ILoadBalancer $lb The load balancer. Prior to 1.35, the first parameter was the + * master name. * @return float|bool UNIX timestamp when client last touched the DB; false if not on record - * @since 1.28 + * @since 1.35 */ - public function getTouched( $dbName ) { - return $this->store->get( $this->getTouchedKey( $this->store, $dbName ) ); + public function getTouched( ILoadBalancer $lb ) { + $masterName = $lb->getServerName( $lb->getWriterIndex() ); + return $this->store->get( $this->getTouchedKey( $this->store, $masterName ) ); } /** * @param BagOStuff $store - * @param string $dbName + * @param string $masterName * @return string */ - private function getTouchedKey( BagOStuff $store, $dbName ) { - return $store->makeGlobalKey( __CLASS__, 'mtime', $this->clientId, $dbName ); + private function getTouchedKey( BagOStuff $store, $masterName ) { + return $store->makeGlobalKey( __CLASS__, 'mtime', $this->clientId, $masterName ); } /** diff --git a/includes/libs/rdbms/lbfactory/ILBFactory.php b/includes/libs/rdbms/lbfactory/ILBFactory.php index 47d1540f846..92af3d6a078 100644 --- a/includes/libs/rdbms/lbfactory/ILBFactory.php +++ b/includes/libs/rdbms/lbfactory/ILBFactory.php @@ -330,10 +330,10 @@ interface ILBFactory { public function commitAndWaitForReplication( $fname, $ticket, array $opts = [] ); /** - * @param string $dbName DB master name (e.g. "db1052") + * @param DatabaseDomain|string|bool $domain Domain ID, or false for the current domain * @return float|bool UNIX timestamp when client last touched the DB or false if not recent */ - public function getChronologyProtectorTouched( $dbName ); + public function getChronologyProtectorTouched( $domain = false ); /** * Disable the ChronologyProtector for all load balancers diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php index a4f252220d5..ff9542c5aea 100644 --- a/includes/libs/rdbms/lbfactory/LBFactory.php +++ b/includes/libs/rdbms/lbfactory/LBFactory.php @@ -530,8 +530,8 @@ abstract class LBFactory implements ILBFactory { return $waitSucceeded; } - public function getChronologyProtectorTouched( $dbName ) { - return $this->getChronologyProtector()->getTouched( $dbName ); + public function getChronologyProtectorTouched( $domain = false ) { + return $this->getChronologyProtector()->getTouched( $this->getMainLB( $domain ) ); } public function disableChronologyProtection() { diff --git a/tests/phpunit/includes/db/LBFactoryTest.php b/tests/phpunit/includes/db/LBFactoryTest.php index 17034102f85..b15318c8fb4 100644 --- a/tests/phpunit/includes/db/LBFactoryTest.php +++ b/tests/phpunit/includes/db/LBFactoryTest.php @@ -799,4 +799,21 @@ class LBFactoryTest extends MediaWikiTestCase { $this->assertEquals( 'realdb', $lb->resolveDomainID( 'alias-db' ) ); $this->assertEquals( "realdb-realprefix_", $lb->resolveDomainID( "alias-db-prefix_" ) ); } + + /** + * @covers \Wikimedia\Rdbms\ChronologyProtector + * @covers \Wikimedia\Rdbms\LBFactory + */ + public function testGetChronologyProtectorTouched() { + $store = new HashBagOStuff; + $lbFactory = $this->newLBFactoryMulti( [ + 'memStash' => $store + ] ); + $lbFactory->setRequestInfo( [ 'ChronologyClientId' => 'ii' ] ); + $key = $store->makeGlobalKey( ChronologyProtector::class, + 'mtime', 'ii', 'test-db1' ); + $store->set( $key, 2 ); + $touched = $lbFactory->getChronologyProtectorTouched(); + $this->assertEquals( 2, $touched ); + } }