rdbms: add replica server counting methods to ILoadBalancer
authorAaron Schulz <aschulz@wikimedia.org>
Tue, 18 Jun 2019 21:12:06 +0000 (22:12 +0100)
committerAaron Schulz <aschulz@wikimedia.org>
Thu, 20 Jun 2019 11:47:23 +0000 (12:47 +0100)
This is slightly more robust and makes the intent much clearer
than random calling code checking getServerCount() all over the
place. In addition, this yields better separation of concern.

Also, cleanup the LoadBalancer constructer a bit and make the
validation a bit stricter.

Make some server index comparisons strict while at it.

Change-Id: Icc1a35bd65c6862ff81faa3ab9b2aa7cafe29443

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 a2202b6..4c7bc2d 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 62f4582..d075c49 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
                }
 
@@ -1960,11 +1970,13 @@ class LoadBalancer implements ILoadBalancer {
        }
 
        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
                        ]
                ];