Merge "rdbms: add replica server counting methods to ILoadBalancer"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Fri, 21 Jun 2019 20:23:21 +0000 (20:23 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Fri, 21 Jun 2019 20:23:21 +0000 (20:23 +0000)
includes/Revision/RevisionStore.php
includes/jobqueue/JobRunner.php
includes/libs/rdbms/ChronologyProtector.php
includes/libs/rdbms/lbfactory/LBFactory.php
includes/libs/rdbms/loadbalancer/ILoadBalancer.php
includes/libs/rdbms/loadbalancer/LoadBalancer.php
tests/phpunit/includes/db/LBFactoryTest.php
tests/phpunit/includes/db/LoadBalancerTest.php

index 00c9d04..faa162a 100644 (file)
@@ -2131,7 +2131,7 @@ class RevisionStore
                // within web requests to certain avoid bugs like T93866 and T94407.
                if ( !$rev
                        && !( $flags & self::READ_LATEST )
-                       && $lb->getServerCount() > 1
+                       && $lb->hasStreamingReplicaServers()
                        && $lb->hasOrMadeRecentMasterChanges()
                ) {
                        $flags = self::READ_LATEST;
index 676659f..454f694 100644 (file)
@@ -535,7 +535,7 @@ class JobRunner implements LoggerAwareInterface {
 
                $time = false;
                $lb = $lbFactory->getMainLB();
-               if ( $syncThreshold !== false && $lb->getServerCount() > 1 ) {
+               if ( $syncThreshold !== false && $lb->hasStreamingReplicaServers() ) {
                        // Generally, there is one master connection to the local DB
                        $dbwSerial = $lb->getAnyOpenConnection( $lb->getWriterIndex() );
                        // We need natively blocking fast locks
index 88bc049..24b5402 100644 (file)
@@ -176,7 +176,7 @@ class ChronologyProtector implements LoggerAwareInterface {
                }
 
                $masterName = $lb->getServerName( $lb->getWriterIndex() );
-               if ( $lb->getServerCount() > 1 ) {
+               if ( $lb->hasStreamingReplicaServers() ) {
                        $pos = $lb->getMasterPos();
                        if ( $pos ) {
                                $this->logger->debug( __METHOD__ . ": LB for '$masterName' has pos $pos\n" );
index f48487a..e20f6de 100644 (file)
@@ -416,12 +416,11 @@ abstract class LBFactory implements ILBFactory {
                // time needed to wait on the next clusters.
                $masterPositions = array_fill( 0, count( $lbs ), false );
                foreach ( $lbs as $i => $lb ) {
-                       if ( $lb->getServerCount() <= 1 ) {
-                               // T29975 - Don't try to wait for replica DBs if there are none
-                               // Prevents permission error when getting master position
-                               continue;
-                       } elseif ( $opts['ifWritesSince']
-                               && $lb->lastMasterChangeTimestamp() < $opts['ifWritesSince']
+                       if ( !$lb->hasStreamingReplicaServers() ) {
+                               continue; // T29975: no replication; avoid getMasterPos() permissions errors
+                       } elseif (
+                               $opts['ifWritesSince'] &&
+                               $lb->lastMasterChangeTimestamp() < $opts['ifWritesSince']
                        ) {
                                continue; // no writes since the last wait
                        }
@@ -675,7 +674,7 @@ abstract class LBFactory implements ILBFactory {
        public function appendShutdownCPIndexAsQuery( $url, $index ) {
                $usedCluster = 0;
                $this->forEachLB( function ( ILoadBalancer $lb ) use ( &$usedCluster ) {
-                       $usedCluster |= ( $lb->getServerCount() > 1 );
+                       $usedCluster |= $lb->hasStreamingReplicaServers();
                } );
 
                if ( !$usedCluster ) {
index 22281c1..f0d071b 100644 (file)
@@ -62,6 +62,8 @@ use InvalidArgumentException;
  *      Note that lag is still possible depending on how wsrep-sync-wait is set server-side.
  *   - Read-only archive clones: set 'is static' in the server configuration maps. This will
  *      treat all such DBs as having 0 lag.
+ *   - Externally updated dataset clones: set 'is static' in the server configuration maps.
+ *      This will treat all such DBs as having 0 lag.
  *   - SQL load balancing proxy: any proxy should handle lag checks on its own, so the 'max lag'
  *      parameter should probably be set to INF in the server configuration maps. This will make
  *      the load balancer ignore whatever it detects as the lag of the logical replica is (which
@@ -148,7 +150,7 @@ interface ILoadBalancer {
        public function redefineLocalDomain( $domain );
 
        /**
-        * Get the index of the reader connection, which may be a replica DB
+        * Get the server index of the reader connection for a given group
         *
         * This takes into account load ratios and lag times. It should return a consistent
         * index during the life time of the load balancer. This initially checks replica DBs
@@ -306,6 +308,8 @@ interface ILoadBalancer {
        public function getMaintenanceConnectionRef( $i, $groups = [], $domain = false, $flags = 0 );
 
        /**
+        * Get the server index of the master server
+        *
         * @return int
         */
        public function getWriterIndex();
@@ -327,12 +331,44 @@ interface ILoadBalancer {
        public function isNonZeroLoad( $i );
 
        /**
-        * Get the number of defined servers (not the number of open connections)
+        * Get the number of servers defined in configuration
         *
         * @return int
         */
        public function getServerCount();
 
+       /**
+        * Whether there are any replica servers configured
+        *
+        * This counts both servers using streaming replication from the master server and
+        * servers that just have a clone of the static dataset found on the master server
+        *
+        * @return int
+        * @since 1.34
+        */
+       public function hasReplicaServers();
+
+       /**
+        * Whether any replica servers use streaming replication from the master server
+        *
+        * Generally this is one less than getServerCount(), though it might otherwise
+        * return a lower number if some of the servers are configured with "is static".
+        * That flag is used when both the server has no active replication setup and the
+        * dataset is either read-only or occasionally updated out-of-band. For example,
+        * a script might import a new geographic information dataset each week by writing
+        * it to each server and later directing the application to use the new version.
+        *
+        * It is possible for some replicas to be configured with "is static" but not
+        * others, though it generally should either be set for all or none of the replicas.
+        *
+        * If this returns zero, this means that there is generally no reason to execute
+        * replication wait logic for session consistency and lag reduction.
+        *
+        * @return int
+        * @since 1.34
+        */
+       public function hasStreamingReplicaServers();
+
        /**
         * Get the host name or IP address of the server with the specified index
         *
@@ -581,7 +617,7 @@ interface ILoadBalancer {
        public function forEachOpenReplicaConnection( $callback, array $params = [] );
 
        /**
-        * Get the hostname and lag time of the most-lagged replica DB
+        * Get the hostname and lag time of the most-lagged replica server
         *
         * This is useful for maintenance scripts that need to throttle their updates.
         * May attempt to open connections to replica DBs on the default DB. If there is
index f0e4b4f..3936271 100644 (file)
@@ -166,15 +166,29 @@ class LoadBalancer implements ILoadBalancer {
        const ROUND_ERROR = 'error';
 
        public function __construct( array $params ) {
-               if ( !isset( $params['servers'] ) ) {
-                       throw new InvalidArgumentException( __CLASS__ . ': missing "servers" parameter' );
+               if ( !isset( $params['servers'] ) || !count( $params['servers'] ) ) {
+                       throw new InvalidArgumentException( 'Missing or empty "servers" parameter' );
                }
-               $this->servers = $params['servers'];
-               foreach ( $this->servers as $i => $server ) {
+
+               $listKey = -1;
+               $this->servers = [];
+               $this->genericLoads = [];
+               foreach ( $params['servers'] as $i => $server ) {
+                       if ( ++$listKey !== $i ) {
+                               throw new UnexpectedValueException( 'List expected for "servers" parameter' );
+                       }
                        if ( $i == 0 ) {
-                               $this->servers[$i]['master'] = true;
+                               $server['master'] = true;
                        } else {
-                               $this->servers[$i]['replica'] = true;
+                               $server['replica'] = true;
+                       }
+                       $this->servers[$i] = $server;
+
+                       $this->genericLoads[$i] = $server['load'];
+                       if ( isset( $server['groupLoads'] ) ) {
+                               foreach ( $server['groupLoads'] as $group => $ratio ) {
+                                       $this->groupLoads[$group][$i] = $ratio;
+                               }
                        }
                }
 
@@ -185,17 +199,7 @@ class LoadBalancer implements ILoadBalancer {
 
                $this->waitTimeout = $params['waitTimeout'] ?? self::MAX_WAIT_DEFAULT;
 
-               $this->conns = [
-                       // Connection were transaction rounds may be applied
-                       self::KEY_LOCAL => [],
-                       self::KEY_FOREIGN_INUSE => [],
-                       self::KEY_FOREIGN_FREE => [],
-                       // Auto-committing counterpart connections that ignore transaction rounds
-                       self::KEY_LOCAL_NOROUND => [],
-                       self::KEY_FOREIGN_INUSE_NOROUND => [],
-                       self::KEY_FOREIGN_FREE_NOROUND => []
-               ];
-               $this->genericLoads = [];
+               $this->conns = self::newConnsArray();
                $this->waitForPos = false;
                $this->allowLagged = false;
 
@@ -208,18 +212,6 @@ class LoadBalancer implements ILoadBalancer {
                $this->loadMonitorConfig = $params['loadMonitor'] ?? [ 'class' => 'LoadMonitorNull' ];
                $this->loadMonitorConfig += [ 'lagWarnThreshold' => $this->maxLag ];
 
-               foreach ( $params['servers'] as $i => $server ) {
-                       $this->genericLoads[$i] = $server['load'];
-                       if ( isset( $server['groupLoads'] ) ) {
-                               foreach ( $server['groupLoads'] as $group => $ratio ) {
-                                       if ( !isset( $this->groupLoads[$group] ) ) {
-                                               $this->groupLoads[$group] = [];
-                                       }
-                                       $this->groupLoads[$group][$i] = $ratio;
-                               }
-                       }
-               }
-
                $this->srvCache = $params['srvCache'] ?? new EmptyBagOStuff();
                $this->wanCache = $params['wanCache'] ?? WANObjectCache::newEmpty();
                $this->profiler = $params['profiler'] ?? null;
@@ -256,6 +248,19 @@ class LoadBalancer implements ILoadBalancer {
                $this->ownerId = $params['ownerId'] ?? null;
        }
 
+       private static function newConnsArray() {
+               return [
+                       // Connection were transaction rounds may be applied
+                       self::KEY_LOCAL => [],
+                       self::KEY_FOREIGN_INUSE => [],
+                       self::KEY_FOREIGN_FREE => [],
+                       // Auto-committing counterpart connections that ignore transaction rounds
+                       self::KEY_LOCAL_NOROUND => [],
+                       self::KEY_FOREIGN_INUSE_NOROUND => [],
+                       self::KEY_FOREIGN_FREE_NOROUND => []
+               ];
+       }
+
        public function getLocalDomainID() {
                return $this->localDomain->getId();
        }
@@ -349,7 +354,7 @@ class LoadBalancer implements ILoadBalancer {
 
                # Unset excessively lagged servers
                foreach ( $lags as $i => $lag ) {
-                       if ( $i != 0 ) {
+                       if ( $i !== $this->getWriterIndex() ) {
                                # How much lag this server nominally is allowed to have
                                $maxServerLag = $this->servers[$i]['max lag'] ?? $this->maxLag; // default
                                # Constrain that futher by $maxLag argument
@@ -440,7 +445,7 @@ class LoadBalancer implements ILoadBalancer {
        }
 
        public function getReaderIndex( $group = false, $domain = false ) {
-               if ( count( $this->servers ) == 1 ) {
+               if ( $this->getServerCount() == 1 ) {
                        // Skip the load balancing if there's only one server
                        return $this->getWriterIndex();
                }
@@ -479,7 +484,7 @@ class LoadBalancer implements ILoadBalancer {
 
                // If data seen by queries is expected to reflect the transactions committed as of
                // or after a given replication position then wait for the DB to apply those changes
-               if ( $this->waitForPos && $i != $this->getWriterIndex() && !$this->doWait( $i ) ) {
+               if ( $this->waitForPos && $i !== $this->getWriterIndex() && !$this->doWait( $i ) ) {
                        // Data will be outdated compared to what was expected
                        $laggedReplicaMode = true;
                }
@@ -566,7 +571,7 @@ class LoadBalancer implements ILoadBalancer {
                                        // Any server with less lag than it's 'max lag' param is preferable
                                        $i = $this->getRandomNonLagged( $currentLoads, $domain );
                                }
-                               if ( $i === false && count( $currentLoads ) != 0 ) {
+                               if ( $i === false && count( $currentLoads ) ) {
                                        // All replica DBs lagged. Switch to read-only mode
                                        $this->replLogger->error(
                                                __METHOD__ . ": all replica DBs lagged. Switch to read-only mode" );
@@ -661,7 +666,7 @@ class LoadBalancer implements ILoadBalancer {
                $oldPos = $this->waitForPos;
                try {
                        $this->waitForPos = $pos;
-                       $serverCount = count( $this->servers );
+                       $serverCount = $this->getServerCount();
 
                        $ok = true;
                        for ( $i = 1; $i < $serverCount; $i++ ) {
@@ -722,10 +727,10 @@ class LoadBalancer implements ILoadBalancer {
        }
 
        /**
-        * Wait for a given replica DB to catch up to the master pos stored in $this
+        * Wait for a given replica DB to catch up to the master pos stored in "waitForPos"
         * @param int $index Server index
         * @param bool $open Check the server even if a new connection has to be made
-        * @param int|null $timeout Max seconds to wait; default is "waitTimeout" given to __construct()
+        * @param int|null $timeout Max seconds to wait; default is "waitTimeout"
         * @return bool
         */
        protected function doWait( $index, $open = false, $timeout = null ) {
@@ -826,7 +831,7 @@ class LoadBalancer implements ILoadBalancer {
 
                $domain = $this->resolveDomainID( $domain );
                $flags = $this->sanitizeConnectionFlags( $flags );
-               $masterOnly = ( $i == self::DB_MASTER || $i == $this->getWriterIndex() );
+               $masterOnly = ( $i === self::DB_MASTER || $i === $this->getWriterIndex() );
 
                // Number of connections made before getting the server index and handle
                $priorConnectionsMade = $this->connsOpened;
@@ -862,7 +867,7 @@ class LoadBalancer implements ILoadBalancer {
                }
 
                $this->enforceConnectionFlags( $conn, $flags );
-               if ( $serverIndex == $this->getWriterIndex() ) {
+               if ( $serverIndex === $this->getWriterIndex() ) {
                        // If the load balancer is read-only, perhaps due to replication lag, then master
                        // DB handles will reflect that. Note that Database::assertIsWritableMaster() takes
                        // care of replica DB handles whereas getReadOnlyReason() would cause infinite loops.
@@ -1317,6 +1322,20 @@ class LoadBalancer implements ILoadBalancer {
                return count( $this->servers );
        }
 
+       public function hasReplicaServers() {
+               return ( $this->getServerCount() > 1 );
+       }
+
+       public function hasStreamingReplicaServers() {
+               foreach ( $this->servers as $i => $server ) {
+                       if ( $i !== $this->getWriterIndex() && empty( $server['is static'] ) ) {
+                               return true;
+                       }
+               }
+
+               return false;
+       }
+
        public function getServerName( $i ) {
                $name = $this->servers[$i]['hostName'] ?? $this->servers[$i]['host'] ?? '';
 
@@ -1336,7 +1355,7 @@ class LoadBalancer implements ILoadBalancer {
                # master (however unlikely that may be), then we can fetch the position from the replica DB.
                $masterConn = $this->getAnyOpenConnection( $this->getWriterIndex() );
                if ( !$masterConn ) {
-                       $serverCount = count( $this->servers );
+                       $serverCount = $this->getServerCount();
                        for ( $i = 1; $i < $serverCount; $i++ ) {
                                $conn = $this->getAnyOpenConnection( $i );
                                if ( $conn ) {
@@ -1364,14 +1383,7 @@ class LoadBalancer implements ILoadBalancer {
                        $conn->close();
                } );
 
-               $this->conns = [
-                       self::KEY_LOCAL => [],
-                       self::KEY_FOREIGN_INUSE => [],
-                       self::KEY_FOREIGN_FREE => [],
-                       self::KEY_LOCAL_NOROUND => [],
-                       self::KEY_FOREIGN_INUSE_NOROUND => [],
-                       self::KEY_FOREIGN_FREE_NOROUND => []
-               ];
+               $this->conns = self::newConnsArray();
                $this->connsOpened = 0;
        }
 
@@ -1785,7 +1797,7 @@ class LoadBalancer implements ILoadBalancer {
        public function getLaggedReplicaMode( $domain = false ) {
                if (
                        // Avoid recursion if there is only one DB
-                       $this->getServerCount() > 1 &&
+                       $this->hasStreamingReplicaServers() &&
                        // Avoid recursion if the (non-zero load) master DB was picked for generic reads
                        $this->genericReadIndex !== $this->getWriterIndex() &&
                        // Stay in lagged replica mode during the load balancer instance lifetime
@@ -1921,20 +1933,18 @@ class LoadBalancer implements ILoadBalancer {
        }
 
        public function getMaxLag( $domain = false ) {
-               $maxLag = -1;
                $host = '';
+               $maxLag = -1;
                $maxIndex = 0;
 
-               if ( $this->getServerCount() <= 1 ) {
-                       return [ $host, $maxLag, $maxIndex ]; // no replication = no lag
-               }
-
-               $lagTimes = $this->getLagTimes( $domain );
-               foreach ( $lagTimes as $i => $lag ) {
-                       if ( $this->genericLoads[$i] > 0 && $lag > $maxLag ) {
-                               $maxLag = $lag;
-                               $host = $this->servers[$i]['host'];
-                               $maxIndex = $i;
+               if ( $this->hasReplicaServers() ) {
+                       $lagTimes = $this->getLagTimes( $domain );
+                       foreach ( $lagTimes as $i => $lag ) {
+                               if ( $this->genericLoads[$i] > 0 && $lag > $maxLag ) {
+                                       $maxLag = $lag;
+                                       $host = $this->servers[$i]['host'];
+                                       $maxIndex = $i;
+                               }
                        }
                }
 
@@ -1942,7 +1952,7 @@ class LoadBalancer implements ILoadBalancer {
        }
 
        public function getLagTimes( $domain = false ) {
-               if ( $this->getServerCount() <= 1 ) {
+               if ( !$this->hasReplicaServers() ) {
                        return [ $this->getWriterIndex() => 0 ]; // no replication = no lag
                }
 
@@ -1975,11 +1985,13 @@ class LoadBalancer implements ILoadBalancer {
         * @deprecated Since 1.34 Use IDatabase::getLag() instead
         */
        public function safeGetLag( IDatabase $conn ) {
-               if ( $this->getServerCount() <= 1 ) {
-                       return 0;
-               } else {
-                       return $conn->getLag();
+               if ( $conn->getLBInfo( 'is static' ) ) {
+                       return 0; // static dataset
+               } elseif ( $conn->getLBInfo( 'serverIndex' ) == $this->getWriterIndex() ) {
+                       return 0; // this is the master
                }
+
+               return $conn->getLag();
        }
 
        public function safeWaitForMasterPos( IDatabase $conn, $pos = false, $timeout = null ) {
index f7007e7..123b080 100644 (file)
@@ -302,6 +302,8 @@ class LBFactoryTest extends MediaWikiTestCase {
                        ->getMock();
                $lb1->method( 'getConnection' )->willReturn( $mockDB1 );
                $lb1->method( 'getServerCount' )->willReturn( 2 );
+               $lb1->method( 'hasReplicaServers' )->willReturn( true );
+               $lb1->method( 'hasStreamingReplicaServers' )->willReturn( true );
                $lb1->method( 'getAnyOpenConnection' )->willReturn( $mockDB1 );
                $lb1->method( 'hasOrMadeRecentMasterChanges' )->will( $this->returnCallback(
                                function () use ( $mockDB1 ) {
@@ -327,6 +329,8 @@ class LBFactoryTest extends MediaWikiTestCase {
                        ->getMock();
                $lb2->method( 'getConnection' )->willReturn( $mockDB2 );
                $lb2->method( 'getServerCount' )->willReturn( 2 );
+               $lb2->method( 'hasReplicaServers' )->willReturn( true );
+               $lb2->method( 'hasStreamingReplicaServers' )->willReturn( true );
                $lb2->method( 'getAnyOpenConnection' )->willReturn( $mockDB2 );
                $lb2->method( 'hasOrMadeRecentMasterChanges' )->will( $this->returnCallback(
                        function () use ( $mockDB2 ) {
@@ -373,6 +377,8 @@ class LBFactoryTest extends MediaWikiTestCase {
                        ->disableOriginalConstructor()
                        ->getMock();
                $lb1->method( 'getServerCount' )->willReturn( 2 );
+               $lb1->method( 'hasReplicaServers' )->willReturn( true );
+               $lb1->method( 'hasStreamingReplicaServers' )->willReturn( true );
                $lb1->method( 'getServerName' )->with( 0 )->willReturn( 'master1' );
                $lb1->expects( $this->once() )
                        ->method( 'waitFor' )->with( $this->equalTo( $m1Pos ) );
@@ -381,6 +387,8 @@ class LBFactoryTest extends MediaWikiTestCase {
                        ->disableOriginalConstructor()
                        ->getMock();
                $lb2->method( 'getServerCount' )->willReturn( 2 );
+               $lb2->method( 'hasReplicaServers' )->willReturn( true );
+               $lb2->method( 'hasStreamingReplicaServers' )->willReturn( true );
                $lb2->method( 'getServerName' )->with( 0 )->willReturn( 'master2' );
                $lb2->expects( $this->once() )
                        ->method( 'waitFor' )->with( $this->equalTo( $m2Pos ) );
index 169e4bf..1645b85 100644 (file)
@@ -30,6 +30,7 @@ use Wikimedia\TestingAccessWrapper;
 
 /**
  * @group Database
+ * @group medium
  * @covers \Wikimedia\Rdbms\LoadBalancer
  */
 class LoadBalancerTest extends MediaWikiTestCase {
@@ -113,6 +114,10 @@ class LoadBalancerTest extends MediaWikiTestCase {
                // Simulate web request with DBO_TRX
                $lb = $this->newMultiServerLocalLoadBalancer( DBO_TRX );
 
+               $this->assertEquals( 8, $lb->getServerCount() );
+               $this->assertTrue( $lb->hasReplicaServers() );
+               $this->assertTrue( $lb->hasStreamingReplicaServers() );
+
                $dbw = $lb->getConnection( DB_MASTER );
                $this->assertTrue( $dbw->getLBInfo( 'master' ), 'master shows as master' );
                $this->assertEquals(
@@ -260,6 +265,22 @@ class LoadBalancerTest extends MediaWikiTestCase {
                                        'vslow' => 100
                                ],
                                'flags' => $flags
+                       ],
+                       // Replica DB that only has a copy of some static tables
+                       7 => [
+                               'host' => $wgDBserver,
+                               'dbname' => $wgDBname,
+                               'tablePrefix' => $this->dbPrefix(),
+                               'user' => $wgDBuser,
+                               'password' => $wgDBpassword,
+                               'type' => $wgDBtype,
+                               'dbDirectory' => $wgSQLiteDataDir,
+                               'load' => 0,
+                               'groupLoads' => [
+                                       'archive' => 100
+                               ],
+                               'flags' => $flags,
+                               'is static' => true
                        ]
                ];