From: daniel <daniel.kinzler@wikimedia.de>
Date: Tue, 3 Jul 2018 15:46:30 +0000 (+0200)
Subject: Use consistent caching strategy in Revision storage classes
X-Git-Tag: 1.34.0-rc.0~4860^2
X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=53fd8295ff186fc1d01a4f45bd8bbcd5a0bfc1d2

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 1982d028f0..a457c2bf01 100644
--- 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 fb3ef94499..48ffe2c903 100644
--- 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 45a6bafa88..422e1fb553 100644
--- 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 73050e0d3b..e17f855fee 100644
--- 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 6de37af6ba..7ef1182906 100644
--- 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 ) );
 	}