jenkins-bot has submitted this change and it was merged.
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/Usage/Sql/SqlSubscriptionManager.php
M client/includes/Usage/Sql/SqlUsageTracker.php
D client/includes/store/sql/ConnectionManager.php
A client/includes/store/sql/ConsistentReadConnectionManager.php
M client/includes/store/sql/DirectSqlStore.php
M client/tests/phpunit/includes/Usage/Sql/SqlSubscriptionManagerTest.php
M client/tests/phpunit/includes/Usage/Sql/SqlUsageTrackerTest.php
R
client/tests/phpunit/includes/store/sql/ConsistentReadConnectionManagerTest.php
8 files changed, 183 insertions(+), 121 deletions(-)
Approvals:
Thiemo Mättig (WMDE): Looks good to me, approved
jenkins-bot: Verified
diff --git a/client/includes/Usage/Sql/SqlSubscriptionManager.php
b/client/includes/Usage/Sql/SqlSubscriptionManager.php
index 2d3bb1a..c5829fb 100644
--- a/client/includes/Usage/Sql/SqlSubscriptionManager.php
+++ b/client/includes/Usage/Sql/SqlSubscriptionManager.php
@@ -7,7 +7,7 @@
use Exception;
use InvalidArgumentException;
use ResultWrapper;
-use Wikibase\Client\Store\Sql\ConnectionManager;
+use Wikibase\Client\Store\Sql\ConsistentReadConnectionManager;
use Wikibase\Client\Usage\SubscriptionManager;
use Wikibase\Client\Usage\UsageTrackerException;
use Wikibase\DataModel\Entity\EntityId;
@@ -23,14 +23,14 @@
class SqlSubscriptionManager implements SubscriptionManager {
/**
- * @var ConnectionManager
+ * @var ConsistentReadConnectionManager
*/
private $connectionManager;
/**
- * @param ConnectionManager $connectionManager
+ * @param ConsistentReadConnectionManager $connectionManager
*/
- public function __construct( ConnectionManager $connectionManager ) {
+ public function __construct( ConsistentReadConnectionManager
$connectionManager ) {
$this->connectionManager = $connectionManager;
}
diff --git a/client/includes/Usage/Sql/SqlUsageTracker.php
b/client/includes/Usage/Sql/SqlUsageTracker.php
index 829c5ae..edc9417 100644
--- a/client/includes/Usage/Sql/SqlUsageTracker.php
+++ b/client/includes/Usage/Sql/SqlUsageTracker.php
@@ -8,7 +8,7 @@
use Exception;
use InvalidArgumentException;
use Iterator;
-use Wikibase\Client\Store\Sql\ConnectionManager;
+use Wikibase\Client\Store\Sql\ConsistentReadConnectionManager;
use Wikibase\Client\Usage\EntityUsage;
use Wikibase\Client\Usage\PageEntityUsages;
use Wikibase\Client\Usage\UsageLookup;
@@ -31,7 +31,7 @@
private $idParser;
/**
- * @var ConnectionManager
+ * @var ConsistentReadConnectionManager
*/
private $connectionManager;
@@ -42,9 +42,9 @@
/**
* @param EntityIdParser $idParser
- * @param ConnectionManager $connectionManager
+ * @param ConsistentReadConnectionManager $connectionManager
*/
- public function __construct( EntityIdParser $idParser,
ConnectionManager $connectionManager ) {
+ public function __construct( EntityIdParser $idParser,
ConsistentReadConnectionManager $connectionManager ) {
$this->idParser = $idParser;
$this->connectionManager = $connectionManager;
}
diff --git a/client/includes/store/sql/ConnectionManager.php
b/client/includes/store/sql/ConnectionManager.php
deleted file mode 100644
index 000da2f..0000000
--- a/client/includes/store/sql/ConnectionManager.php
+++ /dev/null
@@ -1,95 +0,0 @@
-<?php
-
-namespace Wikibase\Client\Store\Sql;
-
-use DatabaseBase;
-use IDatabase;
-use InvalidArgumentException;
-use LoadBalancer;
-
-/**
- * Database connection manager.
- *
- * @license GPL 2+
- * @author Daniel Kinzler
- */
-class ConnectionManager {
-
- /**
- * @var LoadBalancer
- */
- private $loadBalancer;
-
- /**
- * The symbolic name of the target database, or false for the local
wiki's database.
- *
- * @var string|false
- */
- private $dbName;
-
- /**
- * @param LoadBalancer $loadBalancer
- * @param string|false $dbName Optional, defaults to current wiki.
- * This follows the convention for database names used by
$loadBalancer.
- */
- public function __construct( LoadBalancer $loadBalancer, $dbName =
false ) {
- if ( !is_string( $dbName ) && $dbName !== false ) {
- throw new InvalidArgumentException( '$dbName must be a
string, or false.' );
- }
-
- $this->loadBalancer = $loadBalancer;
- $this->dbName = $dbName;
- }
-
- /**
- * @return DatabaseBase
- */
- public function getReadConnection() {
- return $this->loadBalancer->getConnection( DB_READ, array(),
$this->dbName );
- }
-
- /**
- * @return DatabaseBase
- */
- private function getWriteConnection() {
- return $this->loadBalancer->getConnection( DB_WRITE, array(),
$this->dbName );
- }
-
- /**
- * @param DatabaseBase $db
- */
- public function releaseConnection( IDatabase $db ) {
- $this->loadBalancer->reuseConnection( $db );
- }
-
- /**
- * @param string $fname
- *
- * @return DatabaseBase
- */
- public function beginAtomicSection( $fname ) {
- $db = $this->getWriteConnection();
- $db->startAtomic( $fname );
- return $db;
- }
-
- /**
- * @param DatabaseBase $db
- * @param string $fname
- */
- public function commitAtomicSection( IDatabase $db, $fname ) {
- $db->endAtomic( $fname );
- $this->releaseConnection( $db );
- }
-
- /**
- * @param DatabaseBase $db
- * @param string $fname
- */
- public function rollbackAtomicSection( IDatabase $db, $fname ) {
- //FIXME: there does not seem to be a clean way to roll back an
atomic section?!
- $db->rollback( $fname, 'flush' );
- $this->releaseConnection( $db );
- }
-
-}
diff --git a/client/includes/store/sql/ConsistentReadConnectionManager.php
b/client/includes/store/sql/ConsistentReadConnectionManager.php
new file mode 100644
index 0000000..ab8d6b5
--- /dev/null
+++ b/client/includes/store/sql/ConsistentReadConnectionManager.php
@@ -0,0 +1,139 @@
+<?php
+
+namespace Wikibase\Client\Store\Sql;
+
+use DatabaseBase;
+use IDatabase;
+use InvalidArgumentException;
+use LoadBalancer;
+
+/**
+ * Database connection manager.
+ *
+ * This manages access to master and slave databases. It also manages state
that indicates whether
+ * the slave databases are possibly outdated after a write operation, and thus
the master database
+ * should be used for subsequent read operations.
+ *
+ * @note: Services that access overlapping sets of database tables, or
interact with logically
+ * related sets of data in the database, should share a
ConsistentReadConnectionManager. Services accessing
+ * unrelated sets of information may prefer to not share a
ConsistentReadConnectionManager, so they can still
+ * perform read operations against slave databases after a (unrelated, per the
assumption) write
+ * operation to the master database. Generally, sharing a
ConsistentReadConnectionManager improves consistency
+ * (by avoiding race conditions due to replication lag), but can reduce
performance (by directing
+ * more read operations to the master database server).
+ *
+ * @license GPL 2+
+ * @author Daniel Kinzler
+ */
+class ConsistentReadConnectionManager {
+
+ /**
+ * @var LoadBalancer
+ */
+ private $loadBalancer;
+
+ /**
+ * The symbolic name of the target database, or false for the local
wiki's database.
+ *
+ * @var string|false
+ */
+ 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.
+ */
+ public function __construct( LoadBalancer $loadBalancer, $dbName =
false ) {
+ if ( !is_string( $dbName ) && $dbName !== false ) {
+ throw new InvalidArgumentException( '$dbName must be a
string, or false.' );
+ }
+
+ $this->loadBalancer = $loadBalancer;
+ $this->dbName = $dbName;
+ }
+
+ /**
+ * 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 forceMaster() {
+ $this->forceMaster = true;
+ }
+
+ /**
+ * Returns a database connection for reading.
+ *
+ * @note: If forceMaster() or beginAtomicSection() were previously
called on this
+ * ConsistentReadConnectionManager 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_MASTER, array(),
$this->dbName );
+ }
+
+ /**
+ * @param DatabaseBase $db
+ */
+ public function releaseConnection( IDatabase $db ) {
+ $this->loadBalancer->reuseConnection( $db );
+ }
+
+ /**
+ * 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;
+ }
+
+ /**
+ * @param DatabaseBase $db
+ * @param string $fname
+ */
+ public function commitAtomicSection( IDatabase $db, $fname ) {
+ $db->endAtomic( $fname );
+ $this->releaseConnection( $db );
+ }
+
+ /**
+ * @param DatabaseBase $db
+ * @param string $fname
+ */
+ public function rollbackAtomicSection( IDatabase $db, $fname ) {
+ //FIXME: there does not seem to be a clean way to roll back an
atomic section?!
+ $db->rollback( $fname, 'flush' );
+ $this->releaseConnection( $db );
+ }
+
+}
diff --git a/client/includes/store/sql/DirectSqlStore.php
b/client/includes/store/sql/DirectSqlStore.php
index c8dbeb0..ff44a83 100644
--- a/client/includes/store/sql/DirectSqlStore.php
+++ b/client/includes/store/sql/DirectSqlStore.php
@@ -5,7 +5,7 @@
use HashBagOStuff;
use LoadBalancer;
use ObjectCache;
-use Wikibase\Client\Store\Sql\ConnectionManager;
+use Wikibase\Client\Store\Sql\ConsistentReadConnectionManager;
use Wikibase\Client\Store\Sql\PagePropsEntityIdLookup;
use Wikibase\Client\Store\TitleFactory;
use Wikibase\Client\Usage\NullSubscriptionManager;
@@ -174,7 +174,7 @@
if ( $this->useLegacyChangesSubscription ) {
$this->subscriptionManager = new
NullSubscriptionManager();
} else {
- $connectionManager = new ConnectionManager(
+ $connectionManager = new
ConsistentReadConnectionManager(
$this->getRepoLoadBalancer(),
$this->repoWiki
);
@@ -240,7 +240,7 @@
if ( $this->useLegacyUsageIndex ) {
$this->usageTracker = new NullUsageTracker();
} else {
- $connectionManager = new ConnectionManager(
$this->getLocalLoadBalancer() );
+ $connectionManager = new
ConsistentReadConnectionManager( $this->getLocalLoadBalancer() );
$this->usageTracker = new SqlUsageTracker(
$this->entityIdParser, $connectionManager );
}
}
diff --git
a/client/tests/phpunit/includes/Usage/Sql/SqlSubscriptionManagerTest.php
b/client/tests/phpunit/includes/Usage/Sql/SqlSubscriptionManagerTest.php
index e93415c..4368793 100644
--- a/client/tests/phpunit/includes/Usage/Sql/SqlSubscriptionManagerTest.php
+++ b/client/tests/phpunit/includes/Usage/Sql/SqlSubscriptionManagerTest.php
@@ -2,7 +2,7 @@
namespace Wikibase\Client\Tests\Usage\Sql;
-use Wikibase\Client\Store\Sql\ConnectionManager;
+use Wikibase\Client\Store\Sql\ConsistentReadConnectionManager;
use Wikibase\Client\Usage\Sql\SqlSubscriptionManager;
use Wikibase\Client\WikibaseClient;
use Wikibase\DataModel\Entity\ItemId;
@@ -42,7 +42,7 @@
*/
private function getSubscriptionManager() {
return new SqlSubscriptionManager(
- new ConnectionManager( wfGetLB() )
+ new ConsistentReadConnectionManager( wfGetLB() )
);
}
diff --git a/client/tests/phpunit/includes/Usage/Sql/SqlUsageTrackerTest.php
b/client/tests/phpunit/includes/Usage/Sql/SqlUsageTrackerTest.php
index 3cfccf7..1876256 100644
--- a/client/tests/phpunit/includes/Usage/Sql/SqlUsageTrackerTest.php
+++ b/client/tests/phpunit/includes/Usage/Sql/SqlUsageTrackerTest.php
@@ -2,7 +2,7 @@
namespace Wikibase\Client\Tests\Usage\Sql;
-use Wikibase\Client\Store\Sql\ConnectionManager;
+use Wikibase\Client\Store\Sql\ConsistentReadConnectionManager;
use Wikibase\Client\Tests\Usage\UsageLookupContractTester;
use Wikibase\Client\Tests\Usage\UsageTrackerContractTester;
use Wikibase\Client\Usage\Sql\SqlUsageTracker;
@@ -48,7 +48,7 @@
$this->sqlUsageTracker = new SqlUsageTracker(
new BasicEntityIdParser(),
- new ConnectionManager( wfGetLB() )
+ new ConsistentReadConnectionManager( wfGetLB() )
);
$this->trackerTester = new UsageTrackerContractTester(
$this->sqlUsageTracker );
diff --git a/client/tests/phpunit/includes/store/sql/ConnectionManagerTest.php
b/client/tests/phpunit/includes/store/sql/ConsistentReadConnectionManagerTest.php
similarity index 70%
rename from client/tests/phpunit/includes/store/sql/ConnectionManagerTest.php
rename to
client/tests/phpunit/includes/store/sql/ConsistentReadConnectionManagerTest.php
index 64bb5b5..23050e7 100644
--- a/client/tests/phpunit/includes/store/sql/ConnectionManagerTest.php
+++
b/client/tests/phpunit/includes/store/sql/ConsistentReadConnectionManagerTest.php
@@ -2,10 +2,10 @@
namespace Wikibase\Client\Tests\Store\Sql;
-use Wikibase\Client\Store\Sql\ConnectionManager;
+use Wikibase\Client\Store\Sql\ConsistentReadConnectionManager;
/**
- * @covers Wikibase\Client\Store\Sql\ConnectionManager
+ * @covers Wikibase\Client\Store\Sql\ConsistentReadConnectionManager
*
* @group Wikibase
* @group WikibaseClient
@@ -14,7 +14,7 @@
* @licence GNU GPL v2+
* @author DanielKinzler
*/
-class ConnectionManagerTest extends \PHPUnit_Framework_TestCase {
+class ConsistentReadConnectionManagerTest extends \PHPUnit_Framework_TestCase {
private function getConnectionMock() {
$connection = $this->getMockBuilder( 'IDatabase' )
@@ -38,13 +38,27 @@
$lb->expects( $this->once() )
->method( 'getConnection' )
- ->with( DB_READ )
+ ->with( DB_SLAVE )
->will( $this->returnValue( $connection ) );
- $manager = new ConnectionManager( $lb );
+ $manager = new ConsistentReadConnectionManager( $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 ConsistentReadConnectionManager( $lb );
+ $manager->forceMaster();
+ $manager->getReadConnection();
}
public function testReleaseConnection() {
@@ -56,7 +70,7 @@
->with( $connection )
->will( $this->returnValue( null ) );
- $manager = new ConnectionManager( $lb );
+ $manager = new ConsistentReadConnectionManager( $lb );
$manager->releaseConnection( $connection );
}
@@ -64,17 +78,21 @@
$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() )
->method( 'startAtomic' )
->will( $this->returnValue( null ) );
- $manager = new ConnectionManager( $lb );
+ $manager = new ConsistentReadConnectionManager( $lb );
$manager->beginAtomicSection( 'TEST' );
+
+ // Should also ask for a DB_MASTER connection.
+ // This is asserted by the $lb mock.
+ $manager->getReadConnection();
}
public function testCommitAtomicSection() {
@@ -90,7 +108,7 @@
->method( 'endAtomic' )
->will( $this->returnValue( null ) );
- $manager = new ConnectionManager( $lb );
+ $manager = new ConsistentReadConnectionManager( $lb );
$manager->commitAtomicSection( $connection, 'TEST' );
}
@@ -107,7 +125,7 @@
->method( 'rollback' )
->will( $this->returnValue( null ) );
- $manager = new ConnectionManager( $lb );
+ $manager = new ConsistentReadConnectionManager( $lb );
$manager->rollbackAtomicSection( $connection, 'TEST' );
}
--
To view, visit https://gerrit.wikimedia.org/r/189489
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I691cf4cfb505124bf1ac89ce4f1550b9ff1e6f41
Gerrit-PatchSet: 3
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: Adrian Lang <[email protected]>
Gerrit-Reviewer: Aude <[email protected]>
Gerrit-Reviewer: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: Hoo man <[email protected]>
Gerrit-Reviewer: JanZerebecki <[email protected]>
Gerrit-Reviewer: Thiemo Mättig (WMDE) <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
