From 70c4255978f853f9cf0f6950da5a1644e756378b Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Fri, 5 Jun 2020 10:29:04 -0700 Subject: [PATCH] filerepo: clean up shared cache keys to avoid key metrics clutter This avoids statsd metrics with names that begin with DB domains Bug: T261534 Change-Id: I69fa4482c639bd49bdc2dec48a8dcdd74d455432 --- includes/filerepo/FileRepo.php | 37 +++++++++------- includes/filerepo/ForeignAPIRepo.php | 10 +++-- includes/filerepo/ForeignDBRepo.php | 24 +---------- includes/filerepo/ForeignDBViaLBRepo.php | 30 +++---------- includes/filerepo/LocalRepo.php | 43 ++++++++++++------- includes/filerepo/file/File.php | 2 +- includes/filerepo/file/ForeignAPIFile.php | 8 ++-- includes/filerepo/file/ForeignDBFile.php | 2 +- .../includes/filerepo/LocalRepoTest.php | 28 ++++++------ 9 files changed, 87 insertions(+), 97 deletions(-) diff --git a/includes/filerepo/FileRepo.php b/includes/filerepo/FileRepo.php index 46bc9ca8ed7..9612db7cd62 100644 --- a/includes/filerepo/FileRepo.php +++ b/includes/filerepo/FileRepo.php @@ -1875,30 +1875,37 @@ class FileRepo { } /** - * Get a key on the primary cache for this repository. - * Returns false if the repository's cache is not accessible at this site. - * The parameters are the parts of the key. + * Get a global, repository-qualified, WAN cache key * - * STUB - * @param mixed ...$args - * @return bool + * This might be called from either the site context of the wiki that owns the repo or + * the site context of another wiki that simply has access to the repo. This returns + * false if the repository's cache is not accessible from the current site context. + * + * @param string $kClassSuffix Key collection name suffix (added to this repo class) + * @param mixed ...$components Additional key components + * @return string|false */ - public function getSharedCacheKey( ...$args ) { + public function getSharedCacheKey( $kClassSuffix, ...$components ) { return false; } /** - * Get a key for this repo in the local cache domain. These cache keys are - * not shared with remote instances of the repo. - * The parameters are the parts of the key. + * Get a site-local, repository-qualified, WAN cache key * - * @param mixed ...$args + * These cache keys are not shared among different site context and thus cannot be + * directly invalidated when repo objects are modified. These are useful when there + * is no accessible global cache or the values depend on the current site context. + * + * @param string $kClassSuffix Key collection name suffix (added to this repo class) + * @param mixed ...$components Additional key components * @return string */ - public function getLocalCacheKey( ...$args ) { - array_unshift( $args, 'filerepo', $this->getName() ); - - return $this->wanCache->makeKey( ...$args ); + public function getLocalCacheKey( $kClassSuffix, ...$components ) { + return $this->wanCache->makeKey( + 'filerepo-' . $kClassSuffix, + $this->getName(), + ...$components + ); } /** diff --git a/includes/filerepo/ForeignAPIRepo.php b/includes/filerepo/ForeignAPIRepo.php index 7b957a8f3f1..a698bb7f945 100644 --- a/includes/filerepo/ForeignAPIRepo.php +++ b/includes/filerepo/ForeignAPIRepo.php @@ -341,9 +341,11 @@ class ForeignAPIRepo extends FileRepo { if ( !$this->canCacheThumbs() ) { $result = null; // can't pass "null" by reference, but it's ok as default value + return $this->getThumbUrl( $name, $width, $height, $result, $params ); } - $key = $this->getLocalCacheKey( 'ForeignAPIRepo', 'ThumbUrl', $name ); + + $key = $this->getLocalCacheKey( 'file-thumb-url', sha1( $name ) ); $sizekey = "$width:$height:$params"; /* Get the array of urls that we already know */ @@ -557,12 +559,12 @@ class ForeignAPIRepo extends FileRepo { /** * HTTP GET request to a mediawiki API (with caching) - * @param string $target Used in cache key creation, mostly + * @param string $attribute Used in cache key creation, mostly * @param array $query The query parameters for the API request * @param int $cacheTTL Time to live for the memcached caching * @return string|null */ - public function httpGetCached( $target, $query, $cacheTTL = 3600 ) { + public function httpGetCached( $attribute, $query, $cacheTTL = 3600 ) { if ( $this->mApiBase ) { $url = wfAppendQuery( $this->mApiBase, $query ); } else { @@ -570,7 +572,7 @@ class ForeignAPIRepo extends FileRepo { } return $this->wanCache->getWithSetCallback( - $this->getLocalCacheKey( static::class, $target, md5( $url ) ), + $this->getLocalCacheKey( $attribute, sha1( $url ) ), $cacheTTL, function ( $curValue, &$ttl ) use ( $url ) { $html = self::httpGet( $url, 'default', [], $mtime ); diff --git a/includes/filerepo/ForeignDBRepo.php b/includes/filerepo/ForeignDBRepo.php index 4f21c3f66fc..128a1d1c804 100644 --- a/includes/filerepo/ForeignDBRepo.php +++ b/includes/filerepo/ForeignDBRepo.php @@ -52,9 +52,6 @@ class ForeignDBRepo extends LocalRepo { /** @var string */ protected $tablePrefix; - /** @var bool */ - protected $hasSharedCache; - /** @var IDatabase */ protected $dbConn; @@ -63,14 +60,12 @@ class ForeignDBRepo extends LocalRepo { /** @var callable */ protected $fileFromRowFactory = [ ForeignDBFile::class, 'newFromRow' ]; - /** @var string */ - private $dbDomain; - /** * @param array|null $info */ public function __construct( $info ) { parent::__construct( $info ); + '@phan-var array $info'; $this->dbType = $info['dbType']; $this->dbServer = $info['dbServer']; @@ -79,7 +74,7 @@ class ForeignDBRepo extends LocalRepo { $this->dbName = $info['dbName']; $this->dbFlags = $info['dbFlags']; $this->tablePrefix = $info['tablePrefix']; - $this->hasSharedCache = $info['hasSharedCache']; + $this->hasAccessibleSharedCache = $info['hasSharedCache']; $dbDomain = new DatabaseDomain( $this->dbName, null, $this->tablePrefix ); $this->dbDomain = $dbDomain->getId(); @@ -117,21 +112,6 @@ class ForeignDBRepo extends LocalRepo { }; } - /** - * @return bool - */ - private function hasSharedCache() { - return $this->hasSharedCache; - } - - public function getSharedCacheKey( ...$args ) { - if ( $this->hasSharedCache() ) { - return $this->wanCache->makeGlobalKey( $this->dbDomain, ...$args ); - } else { - return false; - } - } - protected function assertWritableRepo() { throw new MWException( static::class . ': write operations are not supported.' ); } diff --git a/includes/filerepo/ForeignDBViaLBRepo.php b/includes/filerepo/ForeignDBViaLBRepo.php index 2e4ebae54ac..ddd398839ed 100644 --- a/includes/filerepo/ForeignDBViaLBRepo.php +++ b/includes/filerepo/ForeignDBViaLBRepo.php @@ -30,34 +30,28 @@ use Wikimedia\Rdbms\ILoadBalancer; * @ingroup FileRepo */ class ForeignDBViaLBRepo extends LocalRepo { - /** @var string */ - protected $wiki; - /** @var array */ protected $fileFactory = [ ForeignDBFile::class, 'newFromTitle' ]; /** @var array */ protected $fileFromRowFactory = [ ForeignDBFile::class, 'newFromRow' ]; - /** @var bool */ - protected $hasSharedCache; - /** * @param array|null $info */ public function __construct( $info ) { parent::__construct( $info ); '@phan-var array $info'; - $this->wiki = $info['wiki']; - $this->hasSharedCache = $info['hasSharedCache']; + $this->dbDomain = $info['wiki']; + $this->hasAccessibleSharedCache = $info['hasSharedCache']; } public function getMasterDB() { - return $this->getDBLoadBalancer()->getConnectionRef( DB_MASTER, [], $this->wiki ); + return $this->getDBLoadBalancer()->getConnectionRef( DB_MASTER, [], $this->dbDomain ); } public function getReplicaDB() { - return $this->getDBLoadBalancer()->getConnectionRef( DB_REPLICA, [], $this->wiki ); + return $this->getDBLoadBalancer()->getConnectionRef( DB_REPLICA, [], $this->dbDomain ); } /** @@ -65,7 +59,7 @@ class ForeignDBViaLBRepo extends LocalRepo { */ protected function getDBFactory() { return function ( $index ) { - return $this->getDBLoadBalancer()->getConnectionRef( $index, [], $this->wiki ); + return $this->getDBLoadBalancer()->getConnectionRef( $index, [], $this->dbDomain ); }; } @@ -75,19 +69,7 @@ class ForeignDBViaLBRepo extends LocalRepo { protected function getDBLoadBalancer() { $lbFactory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory(); - return $lbFactory->getMainLB( $this->wiki ); - } - - private function hasSharedCache() { - return $this->hasSharedCache; - } - - public function getSharedCacheKey( ...$args ) { - if ( $this->hasSharedCache() ) { - return $this->wanCache->makeGlobalKey( $this->wiki, ...$args ); - } else { - return false; - } + return $lbFactory->getMainLB( $this->dbDomain ); } protected function assertWritableRepo() { diff --git a/includes/filerepo/LocalRepo.php b/includes/filerepo/LocalRepo.php index f8d1b3843c0..003222f601d 100644 --- a/includes/filerepo/LocalRepo.php +++ b/includes/filerepo/LocalRepo.php @@ -48,11 +48,18 @@ class LocalRepo extends FileRepo { /** @var callable */ protected $oldFileFactoryKey = [ OldLocalFile::class, 'newFromKey' ]; + /** @var string DB domain of the repo wiki */ + protected $dbDomain; + /** @var bool Whether shared cache keys are exposed/accessible */ + protected $hasAccessibleSharedCache; + public function __construct( array $info = null ) { parent::__construct( $info ); - $this->hasSha1Storage = isset( $info['storageLayout'] ) - && $info['storageLayout'] === 'sha1'; + $this->dbDomain = WikiMap::getCurrentWikiDbDomain(); + $this->hasAccessibleSharedCache = true; + + $this->hasSha1Storage = ( $info['storageLayout'] ?? null ) === 'sha1'; if ( $this->hasSha1Storage() ) { $this->backend = new FileBackendDBRepoWrapper( [ @@ -197,9 +204,9 @@ class LocalRepo extends FileRepo { public function checkRedirect( Title $title ) { $title = File::normalizeTitle( $title, 'exception' ); - $memcKey = $this->getSharedCacheKey( 'file_redirect', md5( $title->getDBkey() ) ); + $memcKey = $this->getSharedCacheKey( 'file-redirect', md5( $title->getDBkey() ) ); if ( $memcKey === false ) { - $memcKey = $this->getLocalCacheKey( 'file_redirect', md5( $title->getDBkey() ) ); + $memcKey = $this->getLocalCacheKey( 'file-redirect', md5( $title->getDBkey() ) ); $expiry = 300; // no invalidation, 5 minutes } else { $expiry = 86400; // has invalidation, 1 day @@ -494,18 +501,24 @@ class LocalRepo extends FileRepo { } /** - * Get a key on the primary cache for this repository. - * Returns false if the repository's cache is not accessible at this site. - * The parameters are the parts of the key. + * Check whether the repo has a shared cache, accessible from the current site context * - * @param mixed ...$args - * @return string + * @return bool + * @since 1.35 */ - public function getSharedCacheKey( ...$args ) { - return $this->wanCache->makeGlobalKey( - WikiMap::getCurrentWikiDbDomain()->getId(), - ...$args - ); + protected function hasAcessibleSharedCache() { + return $this->hasAccessibleSharedCache; + } + + public function getSharedCacheKey( $kClassSuffix, ...$components ) { + return $this->hasAcessibleSharedCache() + ? $this->wanCache->makeGlobalKey( + 'filerepo-' . $kClassSuffix, + $this->dbDomain, + $this->getName(), + ...$components + ) + : false; } /** @@ -515,7 +528,7 @@ class LocalRepo extends FileRepo { * @return void */ public function invalidateImageRedirect( Title $title ) { - $key = $this->getSharedCacheKey( 'file_redirect', md5( $title->getDBkey() ) ); + $key = $this->getSharedCacheKey( 'file-redirect', md5( $title->getDBkey() ) ); if ( $key ) { $this->getMasterDB()->onTransactionPreCommitOrIdle( function () use ( $key ) { diff --git a/includes/filerepo/file/File.php b/includes/filerepo/file/File.php index a6cc7940cc3..18e4f2a908c 100644 --- a/includes/filerepo/file/File.php +++ b/includes/filerepo/file/File.php @@ -2151,7 +2151,7 @@ abstract class File implements IDBAccessObject { if ( $renderUrl ) { $cache = MediaWikiServices::getInstance()->getMainWANObjectCache(); $key = $this->repo->getLocalCacheKey( - 'RemoteFileDescription', + 'file-remote-description', $lang->getCode(), md5( $this->getName() ) ); diff --git a/includes/filerepo/file/ForeignAPIFile.php b/includes/filerepo/file/ForeignAPIFile.php index 1205e34d0bf..b84d91f21e0 100644 --- a/includes/filerepo/file/ForeignAPIFile.php +++ b/includes/filerepo/file/ForeignAPIFile.php @@ -359,9 +359,11 @@ class ForeignAPIFile extends File { private function purgeDescriptionPage() { $services = MediaWikiServices::getInstance(); $url = $this->repo->getDescriptionRenderUrl( - $this->getName(), $services->getContentLanguage()->getCode() ); - $key = $this->repo->getLocalCacheKey( 'RemoteFileDescription', 'url', md5( $url ) ); + $this->getName(), + $services->getContentLanguage()->getCode() + ); + $key = $this->repo->getLocalCacheKey( 'file-remote-description', md5( $url ) ); $services->getMainWANObjectCache()->delete( $key ); } @@ -369,7 +371,7 @@ class ForeignAPIFile extends File { * @param array $options */ public function purgeThumbnails( $options = [] ) { - $key = $this->repo->getLocalCacheKey( 'ForeignAPIRepo', 'ThumbUrl', $this->getName() ); + $key = $this->repo->getLocalCacheKey( 'file-thumb-url', sha1( $this->getName() ) ); MediaWikiServices::getInstance()->getMainWANObjectCache()->delete( $key ); $files = $this->getThumbnails(); diff --git a/includes/filerepo/file/ForeignDBFile.php b/includes/filerepo/file/ForeignDBFile.php index 918f99505a4..411db81faa3 100644 --- a/includes/filerepo/file/ForeignDBFile.php +++ b/includes/filerepo/file/ForeignDBFile.php @@ -123,7 +123,7 @@ class ForeignDBFile extends LocalFile { return $cache->getWithSetCallback( $this->repo->getLocalCacheKey( - 'ForeignFileDescription', + 'file-foreign-description', $lang->getCode(), md5( $this->getName() ), $touched diff --git a/tests/phpunit/includes/filerepo/LocalRepoTest.php b/tests/phpunit/includes/filerepo/LocalRepoTest.php index a3cb93e9e4a..cead31d8663 100644 --- a/tests/phpunit/includes/filerepo/LocalRepoTest.php +++ b/tests/phpunit/includes/filerepo/LocalRepoTest.php @@ -167,21 +167,25 @@ class LocalRepoTest extends MediaWikiIntegrationTestCase { * @covers ::__construct * @covers ::checkRedirect * @covers ::getSharedCacheKey - * @covers ::getLocalCacheKey */ - public function testCheckRedirect_redirect_noWANCache() { - $this->markTestIncomplete( 'WANObjectCache::makeKey is final' ); - - $mockWan = $this->getMockBuilder( WANObjectCache::class ) - ->setConstructorArgs( [ [ 'cache' => new EmptyBagOStuff ] ] ) - ->setMethods( [ 'makeKey' ] ) + public function testCheckRedirectSharedEmptyCache() { + $dbDomain = WikiMap::getCurrentWikiDbDomain()->getId(); + $mockBag = $this->getMockBuilder( EmptyBagOStuff::class ) + ->onlyMethods( [ 'makeKey', 'makeGlobalKey' ] ) ->getMock(); - $mockWan->expects( $this->exactly( 2 ) )->method( 'makeKey' )->withConsecutive( - [ 'file_redirect', md5( 'Redirect' ) ], - [ 'filerepo', 'local', 'file_redirect', md5( 'Redirect' ) ] - )->will( $this->onConsecutiveCalls( false, 'somekey' ) ); + $mockBag->expects( $this->exactly( 0 ) ) + ->method( 'makeKey' ) + ->withConsecutive( + [ 'filerepo-file-redirect', 'local', md5( 'Redirect' ) ] + ); + $mockBag->expects( $this->exactly( 1 ) ) + ->method( 'makeGlobalKey' ) + ->withConsecutive( + [ 'filerepo-file-redirect', $dbDomain, 'local', md5( 'Redirect' ) ] + ); - $repo = $this->newRepo( [ 'wanCache' => $mockWan ] ); + $wanCache = new WANObjectCache( [ 'cache' => $mockBag ] ); + $repo = $this->newRepo( [ 'wanCache' => $wanCache ] ); $this->editPage( 'File:Redirect', '#REDIRECT [[File:Target]]' ); $this->assertEquals( 'File:Target',