Use consistent caching strategy in Revision storage classes

DEPLOYMENT: This changes the cache key for revision
content blobs. Expect a brief rise in ExternalStore hits.

Bug: T198704
Change-Id: Icc2d16bc5a1e27ba4caea49a784ba7aeac15042a

diff --git a/includes/Storage/NameTableStore.php b/includes/Storage/NameTableStore.php
index 1982d02..a457c2b 100644 (file)
--- a/includes/Storage/NameTableStore.php
+++ b/includes/Storage/NameTableStore.php
@@ -109,8 +109,20 @@ class NameTableStore {
                return $this->loadBalancer->getConnection( $index, [], $this->wikiId, $flags );
        }
 
+       /**
+        * Gets the cache key for names.
+        *
+        * The cache key is constructed based on the wiki ID passed to the constructor, and allows
+        * sharing of name tables cached for a specific database between wikis.
+        *
+        * @return string
+        */
        private function getCacheKey() {
-               return $this->cache->makeKey( 'NameTableSqlStore', $this->table, $this->wikiId );
+               return $this->cache->makeGlobalKey(
+                       'NameTableSqlStore',
+                       $this->table,
+                       $this->loadBalancer->resolveDomainID( $this->wikiId )
+               );
        }
 
        /**

diff --git a/includes/Storage/SqlBlobStore.php b/includes/Storage/SqlBlobStore.php
index fb3ef94..48ffe2c 100644 (file)
--- a/includes/Storage/SqlBlobStore.php
+++ b/includes/Storage/SqlBlobStore.php
@@ -267,8 +267,7 @@ class SqlBlobStore implements IDBAccessObject, BlobStore {
 
                // No negative caching; negative hits on text rows may be due to corrupted replica DBs
                $blob = $this->cache->getWithSetCallback(
-                       // TODO: change key, since this is not necessarily revision text!
-                       $this->cache->makeKey( 'revisiontext', 'textid', $blobAddress ),
+                       $this->getCacheKey( $blobAddress ),
                        $this->getCacheTTL(),
                        function ( $unused, &$ttl, &$setOpts ) use ( $blobAddress, $queryFlags ) {
                                list( $index ) = DBAccessObjectUtils::getDBOptions( $queryFlags );
@@ -356,6 +355,25 @@ class SqlBlobStore implements IDBAccessObject, BlobStore {
                return $blob;
        }
 
+       /**
+        * Get a cache key for a given Blob address.
+        *
+        * The cache key is constructed in a way that allows cached blobs from the same database
+        * to be re-used between wikis. For example, enwiki and frwiki will use the same cache keys
+        * for blobs from the wikidatawiki database.
+        *
+        * @param string $blobAddress
+        * @return string
+        */
+       private function getCacheKey( $blobAddress ) {
+               return $this->cache->makeGlobalKey(
+                       'BlobStore',
+                       'address',
+                       $this->dbLoadBalancer->resolveDomainID( $this->wikiId ),
+                       $blobAddress
+               );
+       }
+
        /**
         * Expand a raw data blob according to the flags given.
         *
@@ -370,7 +388,8 @@ class SqlBlobStore implements IDBAccessObject, BlobStore {
         * @param string|string[] $flags Blob flags, such as 'external' or 'gzip'.
         *   Note that not including 'utf-8' in $flags will cause the data to be decoded
         *   according to the legacy encoding specified via setLegacyEncoding.
-        * @param string|null $cacheKey May be used for caching if given
+        * @param string|null $cacheKey A blob address for use in the cache key. If not given,
+        *   caching is disabled.
         *
         * @return false|string The expanded blob or false on failure
         */
@@ -387,13 +406,10 @@ class SqlBlobStore implements IDBAccessObject, BlobStore {
                                return false;
                        }
 
-                       if ( $cacheKey && $this->wikiId === false ) {
-                               // Make use of the wiki-local revision text cache.
+                       if ( $cacheKey ) {
                                // The cached value should be decompressed, so handle that and return here.
-                               // NOTE: we rely on $this->cache being the right cache for $this->wikiId!
                                return $this->cache->getWithSetCallback(
-                                       // TODO: change key, since this is not necessarily revision text!
-                                       $this->cache->makeKey( 'revisiontext', 'textid', $cacheKey ),
+                                       $this->getCacheKey( $cacheKey ),
                                        $this->getCacheTTL(),
                                        function () use ( $url, $flags ) {
                                                // No negative caching per BlobStore::getBlob()

diff --git a/includes/externalstore/ExternalStoreDB.php b/includes/externalstore/ExternalStoreDB.php
index 45a6baf..422e1fb 100644 (file)
--- a/includes/externalstore/ExternalStoreDB.php
+++ b/includes/externalstore/ExternalStoreDB.php
@@ -194,6 +194,10 @@ class ExternalStoreDB extends ExternalStoreMedium {
                static $externalBlobCache = [];
 
                $cacheID = ( $itemID === false ) ? "$cluster/$id" : "$cluster/$id/";
+
+               $wiki = $this->params['wiki'] ?? false;
+               $cacheID = ( $wiki === false ) ? $cacheID : "$cacheID@$wiki";
+
                if ( isset( $externalBlobCache[$cacheID] ) ) {
                        wfDebugLog( 'ExternalStoreDB-cache',
                                "ExternalStoreDB::fetchBlob cache hit on $cacheID" );

diff --git a/tests/phpunit/includes/RevisionDbTestBase.php b/tests/phpunit/includes/RevisionDbTestBase.php
index 73050e0..e17f855 100644 (file)
--- a/tests/phpunit/includes/RevisionDbTestBase.php
+++ b/tests/phpunit/includes/RevisionDbTestBase.php
@@ -1412,7 +1412,8 @@ abstract class RevisionDbTestBase extends MediaWikiTestCase {
                $rev = $this->testPage->getRevision();
 
                // Clear any previous cache for the revision during creation
-               $key = $cache->makeGlobalKey( RevisionStore::ROW_CACHE_KEY,
+               $key = $cache->makeGlobalKey(
+                       RevisionStore::ROW_CACHE_KEY,
                        $db->getDomainID(),
                        $rev->getPage(),
                        $rev->getId()

diff --git a/tests/phpunit/includes/RevisionTest.php b/tests/phpunit/includes/RevisionTest.php
index 6de37af..7ef1182 100644 (file)
--- a/tests/phpunit/includes/RevisionTest.php
+++ b/tests/phpunit/includes/RevisionTest.php
@@ -895,7 +895,12 @@ class RevisionTest extends MediaWikiTestCase {
                        )
                );
 
-               $cacheKey = $cache->makeKey( 'revisiontext', 'textid', 'tt:7777' );
+               $cacheKey = $cache->makeGlobalKey(
+                       'BlobStore',
+                       'address',
+                       $lb->getLocalDomainID(),
+                       'tt:7777'
+               );
                $this->assertSame( 'AAAABBAAA', $cache->get( $cacheKey ) );
        }