From 4f76b9543a8cec4358cfa15301d0bda38f1b4f26 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Tue, 16 Jul 2019 21:52:32 -0700 Subject: [PATCH] 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 --- .../externalstore/ExternalStoreAccess.php | 16 ++++++++---- includes/externalstore/ExternalStoreDB.php | 7 ------ .../externalstore/ExternalStoreFactory.php | 10 +------- .../externalstore/ExternalStoreMedium.php | 6 +---- .../ExternalStoreFactoryTest.php | 25 ++++++++++++++----- 5 files changed, 32 insertions(+), 32 deletions(-) diff --git a/includes/externalstore/ExternalStoreAccess.php b/includes/externalstore/ExternalStoreAccess.php index 8603cc2697..db15cb1777 100644 --- 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 feb0614d1e..4d70d66f10 100644 --- 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 3f78b8b09e..9998640ddd 100644 --- 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 0cdcf53ee7..dd3d6bac12 100644 --- 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 e63ce59493..8eaecc6ec4 100644 --- a/tests/phpunit/includes/externalstore/ExternalStoreFactoryTest.php +++ b/tests/phpunit/includes/externalstore/ExternalStoreFactoryTest.php @@ -1,5 +1,7 @@ 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 ); -- 2.20.1