externalstore: unbreak writes to non-default storage clusters due to isReadOnly()

Instead of telling ExternalStoreMedium classes the default write stores and using
that to make them read-only, let them be configured via other means. For example,
ExternalStoreMwstore already respects FileBackend::isReadOnly() for each location
(e.g. file backends) and ExternalStoreDB checks LoadBalancer::getReadOnlyMode()
for each location (e.g. DB cluster).

Make ExternalStoreAccess::isReadOnly() take a list of base URLs, default to the
default write stores if not specified.

Bug: T227156
Change-Id: I3161890fb2ccb46d6206628f0cd88f8af9f1688c
Follows-Up: I40c3b5534fc8a31116c4c5eb64ee6e4903a6197a

diff --git a/includes/externalstore/ExternalStoreAccess.php b/includes/externalstore/ExternalStoreAccess.php
index 8603cc2..db15cb1 100644 (file)
--- a/includes/externalstore/ExternalStoreAccess.php
+++ b/includes/externalstore/ExternalStoreAccess.php
@@ -89,7 +89,7 @@ class ExternalStoreAccess implements LoggerAwareInterface {
         *
         * @param string $data
         * @param array $params Map of context parameters; same as ExternalStoreFactory::getStore()
-        * @param string[]|null $tryStores Refer to $wgDefaultExternalStore
+        * @param string[]|null $tryStores Base URLs to try, e.g. [ "DB://cluster1" ]
         * @return string|bool The URL of the stored data item, or false on error
         * @throws ExternalStoreException
         */
@@ -142,16 +142,22 @@ class ExternalStoreAccess implements LoggerAwareInterface {
        }
 
        /**
+        * @param string[]|string|null $storeUrls Base URL(s) to check, e.g. [ "DB://cluster1" ]
         * @return bool Whether all the default insertion stores are marked as read-only
         * @throws ExternalStoreException
         */
-       public function isReadOnly() {
-               $writableStores = $this->storeFactory->getWriteBaseUrls();
-               if ( !$writableStores ) {
+       public function isReadOnly( $storeUrls = null ) {
+               if ( $storeUrls === null ) {
+                       $storeUrls = $this->storeFactory->getWriteBaseUrls();
+               } else {
+                       $storeUrls = is_array( $storeUrls ) ? $storeUrls : [ $storeUrls ];
+               }
+
+               if ( !$storeUrls ) {
                        return false; // no stores exists which can be "read only"
                }
 
-               foreach ( $writableStores as $storeUrl ) {
+               foreach ( $storeUrls as $storeUrl ) {
                        $store = $this->storeFactory->getStoreForUrl( $storeUrl );
                        $location = $this->storeFactory->getStoreLocationFromUrl( $storeUrl );
                        if ( $store !== false && !$store->isReadOnly( $location ) ) {

diff --git a/includes/externalstore/ExternalStoreDB.php b/includes/externalstore/ExternalStoreDB.php
index feb0614..4d70d66 100644 (file)
--- a/includes/externalstore/ExternalStoreDB.php
+++ b/includes/externalstore/ExternalStoreDB.php
@@ -156,13 +156,6 @@ class ExternalStoreDB extends ExternalStoreMedium {
                $lb = $this->getLoadBalancer( $cluster );
                $domainId = $this->getDomainId( $lb->getServerInfo( $lb->getWriterIndex() ) );
 
-               if ( !in_array( $cluster, $this->writableLocations, true ) ) {
-                       $this->logger->debug( "read only external store\n" );
-                       $lb->allowLagged( true );
-               } else {
-                       $this->logger->debug( "writable external store\n" );
-               }
-
                $db = $lb->getConnectionRef( DB_REPLICA, [], $domainId );
                $db->clearFlag( DBO_TRX ); // sanity
 

diff --git a/includes/externalstore/ExternalStoreFactory.php b/includes/externalstore/ExternalStoreFactory.php
index 3f78b8b..9998640 100644 (file)
--- a/includes/externalstore/ExternalStoreFactory.php
+++ b/includes/externalstore/ExternalStoreFactory.php
@@ -55,7 +55,7 @@ class ExternalStoreFactory implements LoggerAwareInterface {
        }
 
        /**
-        * @return string[] List of base URLs for writes, e.g. [ "DB://cluster1" ]
+        * @return string[] List of default base URLs for writes, e.g. [ "DB://cluster1" ]
         * @since 1.34
         */
        public function getWriteBaseUrls() {
@@ -88,14 +88,6 @@ class ExternalStoreFactory implements LoggerAwareInterface {
                        $params['domain'] = $this->localDomainId; // default
                        $params['isDomainImplicit'] = true; // b/c for ExternalStoreDB
                }
-               $params['writableLocations'] = [];
-               // Determine the locations for this protocol/store still receiving writes
-               foreach ( $this->writeBaseUrls as $storeUrl ) {
-                       list( $storeProto, $storePath ) = self::splitStorageUrl( $storeUrl );
-                       if ( $protoLowercase === strtolower( $storeProto ) ) {
-                               $params['writableLocations'][] = $storePath;
-                       }
-               }
                // @TODO: ideally, this class should not hardcode what classes need what backend factory
                // objects. For now, inject the factory instances into __construct() for those that do.
                if ( $protoLowercase === 'db' ) {

diff --git a/includes/externalstore/ExternalStoreMedium.php b/includes/externalstore/ExternalStoreMedium.php
index 0cdcf53..dd3d6ba 100644 (file)
--- a/includes/externalstore/ExternalStoreMedium.php
+++ b/includes/externalstore/ExternalStoreMedium.php
@@ -42,8 +42,6 @@ abstract class ExternalStoreMedium implements LoggerAwareInterface {
        protected $dbDomain;
        /** @var bool Whether this was factoried with an explicit DB domain */
        protected $isDbDomainExplicit;
-       /** @var string[] Writable locations */
-       protected $writableLocations = [];
 
        /** @var LoggerInterface */
        protected $logger;
@@ -51,7 +49,6 @@ abstract class ExternalStoreMedium implements LoggerAwareInterface {
        /**
         * @param array $params Usage context options for this instance:
         *   - domain: the DB domain ID of the wiki the content is for [required]
-        *   - writableLocations: locations that are writable [required]
         *   - logger: LoggerInterface instance [optional]
         *   - isDomainImplicit: whether this was factoried without an explicit DB domain [optional]
         */
@@ -65,7 +62,6 @@ abstract class ExternalStoreMedium implements LoggerAwareInterface {
                }
 
                $this->logger = $params['logger'] ?? new NullLogger();
-               $this->writableLocations = $params['writableLocations'] ?? [];
        }
 
        public function setLogger( LoggerInterface $logger ) {
@@ -118,6 +114,6 @@ abstract class ExternalStoreMedium implements LoggerAwareInterface {
         * @since 1.31
         */
        public function isReadOnly( $location ) {
-               return !in_array( $location, $this->writableLocations, true );
+               return false;
        }
 }

diff --git a/tests/phpunit/includes/externalstore/ExternalStoreFactoryTest.php b/tests/phpunit/includes/externalstore/ExternalStoreFactoryTest.php
index e63ce59..8eaecc6 100644 (file)
--- a/tests/phpunit/includes/externalstore/ExternalStoreFactoryTest.php
+++ b/tests/phpunit/includes/externalstore/ExternalStoreFactoryTest.php
@@ -1,5 +1,7 @@
 <?php
 
+use Wikimedia\Rdbms\LBFactory;
+
 /**
  * @covers ExternalStoreFactory
  * @covers ExternalStoreAccess
@@ -55,9 +57,18 @@ class ExternalStoreFactoryTest extends MediaWikiTestCase {
         * @covers ExternalStoreFactory::getStore
         */
        public function testStoreFactoryBasic() {
-               $active = [ 'memory' ];
-               $defaults = [ 'memory://cluster1', 'memory://cluster2' ];
+               $active = [ 'memory', 'mwstore' ];
+               $defaults = [ 'memory://cluster1', 'memory://cluster2', 'mwstore://memstore1' ];
                $esFactory = new ExternalStoreFactory( $active, $defaults, 'db-prefix' );
+               $this->setMwGlobals( 'wgFileBackends', [
+                       [
+                               'name' => 'memstore1',
+                               'class' => 'MemoryFileBackend',
+                               'domain' => 'its-all-in-your-head',
+                               'readOnly' => 'reason is a lie',
+                               'lockManager' => 'nullLockManager'
+                       ]
+               ] );
 
                $this->assertEquals( $active, $esFactory->getProtocols() );
                $this->assertEquals( $defaults, $esFactory->getWriteBaseUrls() );
@@ -65,14 +76,16 @@ class ExternalStoreFactoryTest extends MediaWikiTestCase {
                /** @var ExternalStoreMemory $store */
                $store = $esFactory->getStore( 'memory' );
                $this->assertInstanceOf( ExternalStoreMemory::class, $store );
-               $this->assertEquals( false, $store->isReadOnly( 'cluster1' ) );
-               $this->assertEquals( false, $store->isReadOnly( 'cluster2' ) );
-               $this->assertEquals( true, $store->isReadOnly( 'clusterOld' ) );
+               $this->assertFalse( $store->isReadOnly( 'cluster1' ), "Location is writable" );
+               $this->assertFalse( $store->isReadOnly( 'cluster2' ), "Location is writable" );
+
+               $mwStore = $esFactory->getStore( 'mwstore' );
+               $this->assertTrue( $mwStore->isReadOnly( 'memstore1' ), "Location is read-only" );
 
                $lb = $this->getMockBuilder( \Wikimedia\Rdbms\LoadBalancer::class )
                        ->disableOriginalConstructor()->getMock();
                $lb->expects( $this->any() )->method( 'getReadOnlyReason' )->willReturn( 'Locked' );
-               $lbFactory = $this->getMockBuilder( \Wikimedia\Rdbms\LBFactory::class )
+               $lbFactory = $this->getMockBuilder( LBFactory::class )
                        ->disableOriginalConstructor()->getMock();
                $lbFactory->expects( $this->any() )->method( 'getExternalLB' )->willReturn( $lb );