Daniel Kinzler has uploaded a new change for review.
https://gerrit.wikimedia.org/r/189489
Change subject: ConnectionManager: do not return slave connection after
returning master conenction.
......................................................................
ConnectionManager: do not return slave connection after returning master
conenction.
The idea is that we should never read from a slave after we have written to
master.
Bug: T88985
Change-Id: I691cf4cfb505124bf1ac89ce4f1550b9ff1e6f41
---
M client/includes/store/sql/ConnectionManager.php
M client/tests/phpunit/includes/store/sql/ConnectionManagerTest.php
2 files changed, 57 insertions(+), 7 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/89/189489/1
diff --git a/client/includes/store/sql/ConnectionManager.php
b/client/includes/store/sql/ConnectionManager.php
index 000da2f..f1426e3 100644
--- a/client/includes/store/sql/ConnectionManager.php
+++ b/client/includes/store/sql/ConnectionManager.php
@@ -28,6 +28,11 @@
private $dbName;
/**
+ * @var bool If true, getReadConnection() will also return a DB_MASTER
connection.
+ */
+ private $forceMaster = false;
+
+ /**
* @param LoadBalancer $loadBalancer
* @param string|false $dbName Optional, defaults to current wiki.
* This follows the convention for database names used by
$loadBalancer.
@@ -42,17 +47,35 @@
}
/**
- * @return DatabaseBase
+ * Forces all future calls to getReadConnection() to return a
connection to the master DB.
+ * Use this before performing read operations that are critical for a
future update.
+ * Calling beginAtomicSection() implies a call to forceMaster().
*/
- public function getReadConnection() {
- return $this->loadBalancer->getConnection( DB_READ, array(),
$this->dbName );
+ public function forceMaster() {
+ $this->forceMaster = true;
}
/**
+ * Returns a database connection for reading.
+ *
+ * @note: If forceMaster() or beginAtomicSection() were previously
called on this
+ * ConnectionManager instance, this method will return a connection to
the master database,
+ * to avoid inconsistencies.
+ *
+ * @return DatabaseBase
+ */
+ public function getReadConnection() {
+ $dbIndex = $this->forceMaster ? DB_MASTER : DB_SLAVE;
+ return $this->loadBalancer->getConnection( $dbIndex, array(),
$this->dbName );
+ }
+
+ /**
+ * Returns a connection to the master DB, for updating.
+ *
* @return DatabaseBase
*/
private function getWriteConnection() {
- return $this->loadBalancer->getConnection( DB_WRITE, array(),
$this->dbName );
+ return $this->loadBalancer->getConnection( DB_MASTER, array(),
$this->dbName );
}
/**
@@ -63,11 +86,20 @@
}
/**
+ * Begins an atomic section and returns a database connection to the
master DB, for updating.
+ *
+ * @note: This causes all future calls to getReadConnection() to return
a connection
+ * to the master DB, even after commitAtomicSection() or
rollbackAtomicSection() have
+ * been called.
+ *
* @param string $fname
*
* @return DatabaseBase
*/
public function beginAtomicSection( $fname ) {
+ // Once we have written to master, do not read from slave.
+ $this->forceMaster();
+
$db = $this->getWriteConnection();
$db->startAtomic( $fname );
return $db;
diff --git a/client/tests/phpunit/includes/store/sql/ConnectionManagerTest.php
b/client/tests/phpunit/includes/store/sql/ConnectionManagerTest.php
index 64bb5b5..434632f 100644
--- a/client/tests/phpunit/includes/store/sql/ConnectionManagerTest.php
+++ b/client/tests/phpunit/includes/store/sql/ConnectionManagerTest.php
@@ -38,13 +38,27 @@
$lb->expects( $this->once() )
->method( 'getConnection' )
- ->with( DB_READ )
+ ->with( DB_SLAVE )
->will( $this->returnValue( $connection ) );
$manager = new ConnectionManager( $lb );
$actual = $manager->getReadConnection();
$this->assertSame( $connection, $actual );
+ }
+
+ public function testForceMaster() {
+ $connection = $this->getConnectionMock();
+ $lb = $this->getLoadBalancerMock();
+
+ $lb->expects( $this->once() )
+ ->method( 'getConnection' )
+ ->with( DB_MASTER )
+ ->will( $this->returnValue( $connection ) );
+
+ $manager = new ConnectionManager( $lb );
+ $manager->forceMaster();
+ $manager->getReadConnection();
}
public function testReleaseConnection() {
@@ -64,9 +78,9 @@
$connection = $this->getConnectionMock();
$lb = $this->getLoadBalancerMock( );
- $lb->expects( $this->once() )
+ $lb->expects( $this->exactly( 2 ) )
->method( 'getConnection' )
- ->with( DB_WRITE )
+ ->with( DB_MASTER )
->will( $this->returnValue( $connection ) );
$connection->expects( $this->once() )
@@ -75,6 +89,10 @@
$manager = new ConnectionManager( $lb );
$manager->beginAtomicSection( 'TEST' );
+
+ // Should also ask for a DB_MASTER connection.
+ // This is asserted by the $lb mock.
+ $manager->getReadConnection();
}
public function testCommitAtomicSection() {
--
To view, visit https://gerrit.wikimedia.org/r/189489
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I691cf4cfb505124bf1ac89ce4f1550b9ff1e6f41
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Daniel Kinzler <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
