From bac2fa4503ba0627ebf1647173cd52cfbdcaff25 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Sun, 11 Sep 2016 14:57:09 -0700 Subject: [PATCH] Various dependency injection cleanups to LoadBalancer * Inject wfWikiID() and MWExceptionHandler into LoadBalancer. * Factor out LBFactory duplication into baseLoadBalancerParams(). * Remove $wgDBtype hack. Presumably, sites with others DBs would not have multiple servers configured if does not work anyway. * Make use of injected TransactionProfiler rather than calling Profiler::instance()->getTransactionProfiler(). * Avoid use of trivial wfSplitWikiID() function. * Make DBConnRef enforce its arguments more strongly and optimize getWiki() to avoid causing a connection attempt. * Avoid deprecated method call in LBFactory::destroyInstance(). Change-Id: If134b62d4f48cd68cb48ccbe149e72f12aa26819 --- includes/db/CloneDatabase.php | 5 +- includes/db/DBConnRef.php | 15 +++++- includes/db/loadbalancer/LBFactory.php | 16 +++++- includes/db/loadbalancer/LBFactoryMulti.php | 17 +++---- includes/db/loadbalancer/LBFactorySimple.php | 16 +++--- includes/db/loadbalancer/LBFactorySingle.php | 7 +-- includes/db/loadbalancer/LoadBalancer.php | 52 ++++++++++++++------ 7 files changed, 83 insertions(+), 45 deletions(-) diff --git a/includes/db/CloneDatabase.php b/includes/db/CloneDatabase.php index 577c98dc72..cab9438780 100644 --- a/includes/db/CloneDatabase.php +++ b/includes/db/CloneDatabase.php @@ -129,8 +129,9 @@ class CloneDatabase { */ public static function changePrefix( $prefix ) { global $wgDBprefix; - wfGetLBFactory()->forEachLB( function( $lb ) use ( $prefix ) { - $lb->forEachOpenConnection( function ( $db ) use ( $prefix ) { + wfGetLBFactory()->forEachLB( function( LoadBalancer $lb ) use ( $prefix ) { + $lb->setDomainPrefix( $prefix ); + $lb->forEachOpenConnection( function ( DatabaseBase $db ) use ( $prefix ) { $db->tablePrefix( $prefix ); } ); } ); diff --git a/includes/db/DBConnRef.php b/includes/db/DBConnRef.php index 9997f2c4b1..8604295385 100644 --- a/includes/db/DBConnRef.php +++ b/includes/db/DBConnRef.php @@ -17,16 +17,22 @@ class DBConnRef implements IDatabase { /** @var array|null */ private $params; + const FLD_INDEX = 0; + const FLD_GROUP = 1; + const FLD_WIKI = 2; + /** * @param LoadBalancer $lb - * @param DatabaseBase|array $conn Connection or (server index, group, wiki ID) array + * @param DatabaseBase|array $conn Connection or (server index, group, wiki ID) */ public function __construct( LoadBalancer $lb, $conn ) { $this->lb = $lb; if ( $conn instanceof DatabaseBase ) { $this->conn = $conn; - } else { + } elseif ( count( $conn ) >= 3 && $conn[self::FLD_WIKI] !== false ) { $this->params = $conn; + } else { + throw new InvalidArgumentException( "Missing lazy connection arguments." ); } } @@ -136,6 +142,11 @@ class DBConnRef implements IDatabase { } public function getWikiID() { + if ( $this->conn === null ) { + // Avoid triggering a connection + return $this->params[self::FLD_WIKI]; + } + return $this->__call( __FUNCTION__, func_get_args() ); } diff --git a/includes/db/loadbalancer/LBFactory.php b/includes/db/loadbalancer/LBFactory.php index cd8dff32a1..df1d94847f 100644 --- a/includes/db/loadbalancer/LBFactory.php +++ b/includes/db/loadbalancer/LBFactory.php @@ -147,7 +147,7 @@ abstract class LBFactory implements DestructibleService { * @deprecated since 1.27, use LBFactory::destroy() */ public static function destroyInstance() { - self::singleton()->destroy(); + MediaWikiServices::getInstance()->getDBLoadBalancerFactory()->destroy(); } /** @@ -620,6 +620,20 @@ abstract class LBFactory implements DestructibleService { } ); } + /** + * Base parameters to LoadBalancer::__construct() + */ + final protected function baseLoadBalancerParams() { + return [ + 'readOnlyReason' => $this->readOnlyReason, + 'trxProfiler' => $this->trxProfiler, + 'srvCache' => $this->srvCache, + 'wanCache' => $this->wanCache, + 'localDomain' => wfWikiID(), + 'errorLogger' => [ MWExceptionHandler::class, 'logException' ] + ]; + } + /** * @param LoadBalancer $lb */ diff --git a/includes/db/loadbalancer/LBFactoryMulti.php b/includes/db/loadbalancer/LBFactoryMulti.php index e8608401a9..d486e1322f 100644 --- a/includes/db/loadbalancer/LBFactoryMulti.php +++ b/includes/db/loadbalancer/LBFactoryMulti.php @@ -311,15 +311,14 @@ class LBFactoryMulti extends LBFactory { * @return LoadBalancer */ private function newLoadBalancer( $template, $loads, $groupLoads, $readOnlyReason ) { - $lb = new LoadBalancer( [ - 'servers' => $this->makeServerArray( $template, $loads, $groupLoads ), - 'loadMonitor' => $this->loadMonitorClass, - 'readOnlyReason' => $readOnlyReason, - 'trxProfiler' => $this->trxProfiler, - 'srvCache' => $this->srvCache, - 'wanCache' => $this->wanCache - ] ); - + $lb = new LoadBalancer( array_merge( + $this->baseLoadBalancerParams(), + [ + 'servers' => $this->makeServerArray( $template, $loads, $groupLoads ), + 'loadMonitor' => $this->loadMonitorClass, + 'readOnlyReason' => $readOnlyReason + ] + ) ); $this->initLoadBalancer( $lb ); return $lb; diff --git a/includes/db/loadbalancer/LBFactorySimple.php b/includes/db/loadbalancer/LBFactorySimple.php index b6fb0d22da..69c4abb5eb 100644 --- a/includes/db/loadbalancer/LBFactorySimple.php +++ b/includes/db/loadbalancer/LBFactorySimple.php @@ -131,15 +131,13 @@ class LBFactorySimple extends LBFactory { } private function newLoadBalancer( array $servers ) { - $lb = new LoadBalancer( [ - 'servers' => $servers, - 'loadMonitor' => $this->loadMonitorClass, - 'readOnlyReason' => $this->readOnlyReason, - 'trxProfiler' => $this->trxProfiler, - 'srvCache' => $this->srvCache, - 'wanCache' => $this->wanCache - ] ); - + $lb = new LoadBalancer( array_merge( + $this->baseLoadBalancerParams(), + [ + 'servers' => $servers, + 'loadMonitor' => $this->loadMonitorClass, + ] + ) ); $this->initLoadBalancer( $lb ); return $lb; diff --git a/includes/db/loadbalancer/LBFactorySingle.php b/includes/db/loadbalancer/LBFactorySingle.php index 14cec0ee7d..de82a1f928 100644 --- a/includes/db/loadbalancer/LBFactorySingle.php +++ b/includes/db/loadbalancer/LBFactorySingle.php @@ -35,12 +35,7 @@ class LBFactorySingle extends LBFactory { public function __construct( array $conf ) { parent::__construct( $conf ); - $this->lb = new LoadBalancerSingle( [ - 'readOnlyReason' => $this->readOnlyReason, - 'trxProfiler' => $this->trxProfiler, - 'srvCache' => $this->srvCache, - 'wanCache' => $this->wanCache - ] + $conf ); + $this->lb = new LoadBalancerSingle( array_merge( $this->baseLoadBalancerParams(), $conf ) ); } /** diff --git a/includes/db/loadbalancer/LoadBalancer.php b/includes/db/loadbalancer/LoadBalancer.php index 42044a7c48..8223ad6a36 100644 --- a/includes/db/loadbalancer/LoadBalancer.php +++ b/includes/db/loadbalancer/LoadBalancer.php @@ -74,6 +74,10 @@ class LoadBalancer { private $trxRoundId = false; /** @var array[] Map of (name => callable) */ private $trxRecurringCallbacks = []; + /** @var string Local Wiki ID and default for selectDB() calls */ + private $localDomain; + /** @var callable Exception logger */ + private $errorLogger; /** @var integer Warn when this many connection are held */ const CONN_HELD_WARN_THRESHOLD = 10; @@ -97,6 +101,8 @@ class LoadBalancer { * - waitTimeout : Maximum time to wait for replicas for consistency [optional] * - srvCache : BagOStuff object [optional] * - wanCache : WANObjectCache object [optional] + * - localDomain: The wiki ID of the "local"/"current" wiki [optional] + * - errorLogger: Callback that takes an Exception and logs it [optional] * @throws MWException */ public function __construct( array $params ) { @@ -107,6 +113,7 @@ class LoadBalancer { $this->mWaitTimeout = isset( $params['waitTimeout'] ) ? $params['waitTimeout'] : self::POS_WAIT_TIMEOUT; + $this->localDomain = isset( $params['localDomain'] ) ? $params['localDomain'] : ''; $this->mReadIndex = -1; $this->mWriteIndex = -1; @@ -161,6 +168,12 @@ class LoadBalancer { } else { $this->trxProfiler = new TransactionProfiler(); } + + $this->errorLogger = isset( $params['errorLogger'] ) + ? $params['errorLogger'] + : function ( Exception $e ) { + trigger_error( E_WARNING, $e->getMessage() ); + }; } /** @@ -240,16 +253,9 @@ class LoadBalancer { * @return bool|int|string */ public function getReaderIndex( $group = false, $wiki = false ) { - global $wgDBtype; - - # @todo FIXME: For now, only go through all this for mysql databases - if ( $wgDBtype != 'mysql' ) { - return $this->getWriterIndex(); - } - if ( count( $this->mServers ) == 1 ) { # Skip the load balancing if there's only one server - return 0; + return $this->getWriterIndex(); } elseif ( $group === false && $this->mReadIndex >= 0 ) { # Shortcut if generic reader exists already return $this->mReadIndex; @@ -273,7 +279,7 @@ class LoadBalancer { throw new MWException( "Empty server array given to LoadBalancer" ); } - # Scale the configured load ratios according to the dynamic load (if the load monitor supports it) + # Scale the configured load ratios according to the dynamic load if supported $this->getLoadMonitor()->scaleLoads( $nonErrorLoads, $group, $wiki ); $laggedReplicaMode = false; @@ -532,7 +538,7 @@ class LoadBalancer { ' with invalid server index' ); } - if ( $wiki === wfWikiID() ) { + if ( $wiki === $this->localDomain ) { $wiki = false; } @@ -581,8 +587,7 @@ class LoadBalancer { if ( $this->connsOpened > $oldConnsOpened ) { $host = $conn->getServer(); $dbname = $conn->getDBname(); - $trxProf = Profiler::instance()->getTransactionProfiler(); - $trxProf->recordConnection( $host, $dbname, $masterOnly ); + $this->trxProfiler->recordConnection( $host, $dbname, $masterOnly ); } if ( $masterOnly ) { @@ -668,6 +673,8 @@ class LoadBalancer { * @return DBConnRef */ public function getLazyConnectionRef( $db, $groups = [], $wiki = false ) { + $wiki = ( $wiki !== false ) ? $wiki : $this->localDomain; + return new DBConnRef( $this, [ $db, $groups, $wiki ] ); } @@ -738,7 +745,8 @@ class LoadBalancer { * @return DatabaseBase */ private function openForeignConnection( $i, $wiki ) { - list( $dbName, $prefix ) = wfSplitWikiID( $wiki ); + list( $dbName, $prefix ) = explode( '-', $wiki, 2 ) + [ '', '' ]; + if ( isset( $this->mConns['foreignUsed'][$i][$wiki] ) ) { // Reuse an already-used connection $conn = $this->mConns['foreignUsed'][$i][$wiki]; @@ -1075,7 +1083,7 @@ class LoadBalancer { try { $conn->commit( $fname, $conn::FLUSHING_ALL_PEERS ); } catch ( DBError $e ) { - MWExceptionHandler::logException( $e ); + call_user_func( $this->errorLogger, $e ); $failures[] = "{$conn->getServer()}: {$e->getMessage()}"; } if ( $restore && $conn->getLBInfo( 'master' ) ) { @@ -1175,7 +1183,7 @@ class LoadBalancer { try { $conn->flushSnapshot( $fname ); } catch ( DBError $e ) { - MWExceptionHandler::logException( $e ); + call_user_func( $this->errorLogger, $e ); $failures[] = "{$conn->getServer()}: {$e->getMessage()}"; } $conn->setTrxEndCallbackSuppression( false ); @@ -1210,7 +1218,7 @@ class LoadBalancer { $conn->flushSnapshot( $fname ); } } catch ( DBError $e ) { - MWExceptionHandler::logException( $e ); + call_user_func( $this->errorLogger, $e ); $failures[] = "{$conn->getServer()}: {$e->getMessage()}"; } if ( $restore ) { @@ -1719,4 +1727,16 @@ class LoadBalancer { } ); } + + /** + * Set a new table prefix for the existing local wiki ID for testing + * + * @param string $prefix + * @since 1.28 + */ + public function setDomainPrefix( $prefix ) { + list( $dbName, ) = explode( '-', $this->localDomain, 2 ); + + $this->localDomain = "{$dbName}-{$prefix}"; + } } -- 2.20.1