Use CacheModule for the external ID cache again

In c1be869e we stopped using CacheModule for this cache, because we
thought the value should not be configurable. However, there are a few
advantages to using CacheModule that we would like to keep:
 * It allows us to expose metrics from the caches returned by
   DefaultMemoryCacheFactory, and also get these in the ExternalIdCache.
 * It allows us to explore persisting the ExternalIdCache as an optional
   optimization, using the normal cache persistence path.

Revert to using CacheModule, and simply document that we do not
recommend changing the maximum size.

This reverts commit c1be869e1d.

Change-Id: Ie3505a6bab4d31b67384feaebcab20388a7ae1b4

Documentation/config-gerrit.txt

@@ -753,6 +753,7 @@ Default is 1024 for most caches, except:
 * `"diff"`: default is `10m` (10 MiB of memory)
 * `"diff_intraline"`: default is `10m` (10 MiB of memory)
 * `"diff_summary"`: default is `10m` (10 MiB of memory)
+* `"external_ids_map"`: default is `1` and should not be changed
 * `"groups"`: default is unlimited
 * `"groups_byname"`: default is unlimited
 * `"groups_byuuid"`: default is unlimited
@@ -854,6 +855,14 @@ Ideally, disk limit of this cache is large enough to cover all changes.
 This should significantly speed up change reindexing, especially
 full offline reindexing.
 
+cache `"external_ids_map"`::
++
+A singleton cache whose sole entry is a map of the parsed representation
+of link:config-accounts.html#external-ids[all current external IDs].
++
+It is not recommended to change the attributes of this cache away from
+the defaults.
+
 cache `"git_tags"`::
 +
 If branch or reference level READ access controls are used, this

java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java

@@ -16,7 +16,6 @@ package com.google.gerrit.server.account.externalids;
 
 import com.google.auto.value.AutoValue;
 import com.google.common.base.Strings;
-import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
 import com.google.common.collect.ImmutableSetMultimap;
@@ -29,6 +28,7 @@ import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
+import com.google.inject.name.Named;
 import java.io.IOException;
 import java.util.Collection;
 import java.util.Set;
@@ -43,31 +43,17 @@ import org.eclipse.jgit.lib.ObjectId;
 class ExternalIdCacheImpl implements ExternalIdCache {
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
+  public static final String CACHE_NAME = "external_ids_map";
+
   private final LoadingCache<ObjectId, AllExternalIds> extIdsByAccount;
   private final ExternalIdReader externalIdReader;
   private final Lock lock;
 
   @Inject
-  ExternalIdCacheImpl(ExternalIdReader externalIdReader) {
+  ExternalIdCacheImpl(
-    this.extIdsByAccount =
+      @Named(CACHE_NAME) LoadingCache<ObjectId, AllExternalIds> extIdsByAccount,
-        CacheBuilder.newBuilder()
+      ExternalIdReader externalIdReader) {
-            // The cached data is potentially pretty large and we are always only interested
+    this.extIdsByAccount = extIdsByAccount;
-            // in the latest value, hence the maximum cache size is set to 1.
-            // This can lead to extra cache loads in case of the following race:
-            // 1. thread 1 reads the notes ref at revision A
-            // 2. thread 2 updates the notes ref to revision B and stores the derived value
-            //    for B in the cache
-            // 3. thread 1 attempts to read the data for revision A from the cache, and misses
-            // 4. later threads attempt to read at B
-            // In this race unneeded reloads are done in step 3 (reload from revision A) and
-            // step 4 (reload from revision B, because the value for revision B was lost when the
-            // reload from revision A was done, since the cache can hold only one entry).
-            // These reloads could be avoided by increasing the cache size to 2. However the race
-            // window between reading the ref and looking it up in the cache is small so that
-            // it's rare that this race happens. Therefore it's not worth to double the memory
-            // usage of this cache, just to avoid this.
-            .maximumSize(1)
-            .build(new Loader(externalIdReader));
     this.externalIdReader = externalIdReader;
     this.lock = new ReentrantLock(true /* fair */);
   }
@@ -159,9 +145,10 @@ class ExternalIdCacheImpl implements ExternalIdCache {
     }
   }
 
-  private static class Loader extends CacheLoader<ObjectId, AllExternalIds> {
+  static class Loader extends CacheLoader<ObjectId, AllExternalIds> {
     private final ExternalIdReader externalIdReader;
 
+    @Inject
     Loader(ExternalIdReader externalIdReader) {
       this.externalIdReader = externalIdReader;
     }

java/com/google/gerrit/server/account/externalids/ExternalIdModule.java

@@ -14,11 +14,34 @@
 
 package com.google.gerrit.server.account.externalids;
 
-import com.google.inject.AbstractModule;
+import com.google.gerrit.server.account.externalids.ExternalIdCacheImpl.AllExternalIds;
+import com.google.gerrit.server.account.externalids.ExternalIdCacheImpl.Loader;
+import com.google.gerrit.server.cache.CacheModule;
+import com.google.inject.TypeLiteral;
+import org.eclipse.jgit.lib.ObjectId;
 
-public class ExternalIdModule extends AbstractModule {
+public class ExternalIdModule extends CacheModule {
   @Override
   protected void configure() {
+    cache(ExternalIdCacheImpl.CACHE_NAME, ObjectId.class, new TypeLiteral<AllExternalIds>() {})
+        // The cached data is potentially pretty large and we are always only interested
+        // in the latest value, hence the maximum cache weight is set to 1.
+        // This can lead to extra cache loads in case of the following race:
+        // 1. thread 1 reads the notes ref at revision A
+        // 2. thread 2 updates the notes ref to revision B and stores the derived value
+        //    for B in the cache
+        // 3. thread 1 attempts to read the data for revision A from the cache, and misses
+        // 4. later threads attempt to read at B
+        // In this race unneeded reloads are done in step 3 (reload from revision A) and
+        // step 4 (reload from revision B, because the value for revision B was lost when the
+        // reload from revision A was done, since the cache can hold only one entry).
+        // These reloads could be avoided by increasing the cache size to 2. However the race
+        // window between reading the ref and looking it up in the cache is small so that
+        // it's rare that this race happens. Therefore it's not worth to double the memory
+        // usage of this cache, just to avoid this.
+        .maximumWeight(1)
+        .loader(Loader.class);
+
     bind(ExternalIdCacheImpl.class);
     bind(ExternalIdCache.class).to(ExternalIdCacheImpl.class);
   }