From 21abf22680a2e46aee9fe440e48c1fa09321e0ba Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Mon, 22 Aug 2016 18:41:05 -0700 Subject: [PATCH] Code cleanups to SqlBagOStuff * Refactor local DB usage check into usesMainDB() method. * Avoid using the db member of DBError instances. Change-Id: I7350f5a471c551492094bfaf545ebc222eb6f7dd --- includes/objectcache/SqlBagOStuff.php | 67 ++++++++++++++++----------- 1 file changed, 39 insertions(+), 28 deletions(-) diff --git a/includes/objectcache/SqlBagOStuff.php b/includes/objectcache/SqlBagOStuff.php index 84936a4db4..9ab28aa44e 100644 --- a/includes/objectcache/SqlBagOStuff.php +++ b/includes/objectcache/SqlBagOStuff.php @@ -21,6 +21,8 @@ * @ingroup Cache */ +use \MediaWiki\MediaWikiServices; + /** * Class to store objects in the database * @@ -276,6 +278,7 @@ class SqlBagOStuff extends BagOStuff { if ( isset( $dataRows[$key] ) ) { // HIT? $row = $dataRows[$key]; $this->debug( "get: retrieved data; expiry time is " . $row->exptime ); + $db = null; try { $db = $this->getDB( $row->serverIndex ); if ( $this->isExpired( $db, $row->exptime ) ) { // MISS @@ -284,7 +287,7 @@ class SqlBagOStuff extends BagOStuff { $values[$key] = $this->unserialize( $db->decodeBlob( $row->value ) ); } } catch ( DBQueryError $e ) { - $this->handleWriteError( $e, $row->serverIndex ); + $this->handleWriteError( $e, $db, $row->serverIndex ); } } else { // MISS $this->debug( 'get: no matching rows' ); @@ -306,10 +309,11 @@ class SqlBagOStuff extends BagOStuff { $result = true; $exptime = (int)$expiry; foreach ( $keysByTable as $serverIndex => $serverKeys ) { + $db = null; try { $db = $this->getDB( $serverIndex ); } catch ( DBError $e ) { - $this->handleWriteError( $e, $serverIndex ); + $this->handleWriteError( $e, $db, $serverIndex ); $result = false; continue; } @@ -342,7 +346,7 @@ class SqlBagOStuff extends BagOStuff { __METHOD__ ); } catch ( DBError $e ) { - $this->handleWriteError( $e, $serverIndex ); + $this->handleWriteError( $e, $db, $serverIndex ); $result = false; } @@ -364,6 +368,7 @@ class SqlBagOStuff extends BagOStuff { protected function cas( $casToken, $key, $value, $exptime = 0 ) { list( $serverIndex, $tableName ) = $this->getTableByKey( $key ); + $db = null; try { $db = $this->getDB( $serverIndex ); $exptime = intval( $exptime ); @@ -394,7 +399,7 @@ class SqlBagOStuff extends BagOStuff { __METHOD__ ); } catch ( DBQueryError $e ) { - $this->handleWriteError( $e, $serverIndex ); + $this->handleWriteError( $e, $db, $serverIndex ); return false; } @@ -404,6 +409,7 @@ class SqlBagOStuff extends BagOStuff { public function delete( $key ) { list( $serverIndex, $tableName ) = $this->getTableByKey( $key ); + $db = null; try { $db = $this->getDB( $serverIndex ); $db->delete( @@ -411,7 +417,7 @@ class SqlBagOStuff extends BagOStuff { [ 'keyname' => $key ], __METHOD__ ); } catch ( DBError $e ) { - $this->handleWriteError( $e, $serverIndex ); + $this->handleWriteError( $e, $db, $serverIndex ); return false; } @@ -420,6 +426,7 @@ class SqlBagOStuff extends BagOStuff { public function incr( $key, $step = 1 ) { list( $serverIndex, $tableName ) = $this->getTableByKey( $key ); + $db = null; try { $db = $this->getDB( $serverIndex ); $step = intval( $step ); @@ -455,7 +462,7 @@ class SqlBagOStuff extends BagOStuff { $newValue = null; } } catch ( DBError $e ) { - $this->handleWriteError( $e, $serverIndex ); + $this->handleWriteError( $e, $db, $serverIndex ); return null; } @@ -473,6 +480,7 @@ class SqlBagOStuff extends BagOStuff { public function changeTTL( $key, $expiry = 0 ) { list( $serverIndex, $tableName ) = $this->getTableByKey( $key ); + $db = null; try { $db = $this->getDB( $serverIndex ); $db->update( @@ -485,7 +493,7 @@ class SqlBagOStuff extends BagOStuff { return false; } } catch ( DBError $e ) { - $this->handleWriteError( $e, $serverIndex ); + $this->handleWriteError( $e, $db, $serverIndex ); return false; } @@ -542,6 +550,7 @@ class SqlBagOStuff extends BagOStuff { */ public function deleteObjectsExpiringBefore( $timestamp, $progressCallback = false ) { for ( $serverIndex = 0; $serverIndex < $this->numServers; $serverIndex++ ) { + $db = null; try { $db = $this->getDB( $serverIndex ); $dbTimestamp = $db->timestamp( $timestamp ); @@ -604,7 +613,7 @@ class SqlBagOStuff extends BagOStuff { } } } catch ( DBError $e ) { - $this->handleWriteError( $e, $serverIndex ); + $this->handleWriteError( $e, $db, $serverIndex ); return false; } } @@ -618,13 +627,14 @@ class SqlBagOStuff extends BagOStuff { */ public function deleteAll() { for ( $serverIndex = 0; $serverIndex < $this->numServers; $serverIndex++ ) { + $db = null; try { $db = $this->getDB( $serverIndex ); for ( $i = 0; $i < $this->shards; $i++ ) { $db->delete( $this->getTableNameByShard( $i ), '*', __METHOD__ ); } } catch ( DBError $e ) { - $this->handleWriteError( $e, $serverIndex ); + $this->handleWriteError( $e, $db, $serverIndex ); return false; } } @@ -694,26 +704,19 @@ class SqlBagOStuff extends BagOStuff { * Handle a DBQueryError which occurred during a write operation. * * @param DBError $exception + * @param IDatabase|null $db DB handle or null if connection failed * @param int $serverIndex * @throws Exception */ - protected function handleWriteError( DBError $exception, $serverIndex ) { - if ( $exception instanceof DBConnectionError ) { + protected function handleWriteError( DBError $exception, IDatabase $db = null, $serverIndex ) { + if ( !$db ) { $this->markServerDown( $exception, $serverIndex ); - } - if ( $exception->db && $exception->db->wasReadOnlyError() ) { - if ( $exception->db->trxLevel() ) { - if ( !$this->serverInfos ) { - // Errors like deadlocks and connection drops already cause rollback. - // For consistency, we have no choice but to throw an error and trigger - // complete rollback if the main DB is also being used as the cache DB. - throw $exception; - } - - try { - $exception->db->rollback( __METHOD__ ); - } catch ( DBError $e ) { - } + } elseif ( $db->wasReadOnlyError() ) { + if ( $db->trxLevel() && $this->usesMainDB() ) { + // Errors like deadlocks and connection drops already cause rollback. + // For consistency, we have no choice but to throw an error and trigger + // complete rollback if the main DB is also being used as the cache DB. + throw $exception; } } @@ -733,7 +736,7 @@ class SqlBagOStuff extends BagOStuff { * @param DBError $exception * @param int $serverIndex */ - protected function markServerDown( $exception, $serverIndex ) { + protected function markServerDown( DBError $exception, $serverIndex ) { unset( $this->conns[$serverIndex] ); // bug T103435 if ( isset( $this->connFailureTimes[$serverIndex] ) ) { @@ -770,11 +773,19 @@ class SqlBagOStuff extends BagOStuff { } } + /** + * @return bool Whether the main DB is used, e.g. wfGetDB( DB_MASTER ) + */ + protected function usesMainDB() { + return !$this->serverInfos; + } + protected function waitForSlaves() { - if ( !$this->serverInfos ) { + if ( $this->usesMainDB() ) { // Main LB is used; wait for any slaves to catch up try { - wfGetLBFactory()->waitForReplication( [ 'wiki' => wfWikiID() ] ); + $lbFactory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory(); + $lbFactory->waitForReplication( [ 'wiki' => wfWikiID() ] ); return true; } catch ( DBReplicationWaitError $e ) { return false; -- 2.20.1