Use expireAfterAccess with a larger cache size for ExternalIdCache

Change-Id: If299a5cc8746cf9e7d59c8e4acf03cb9a29cf02d

Documentation/config-gerrit.txt

@@ -753,7 +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
+* `"external_ids_map"`: default is `2` and should not be changed
 * `"groups"`: default is unlimited
 * `"groups_byname"`: default is unlimited
 * `"groups_byuuid"`: default is unlimited
@@ -858,7 +858,9 @@ 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].
+of link:config-accounts.html#external-ids[all current external IDs]. The
+cache may temporarily contain 2 entries, but the second one is promptly
+expired.
 +
 It is not recommended to change the attributes of this cache away from
 the defaults.

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

@@ -18,6 +18,7 @@ import com.google.gerrit.server.account.externalids.ExternalIdCacheImpl.AllExter
 import com.google.gerrit.server.account.externalids.ExternalIdCacheImpl.Loader;
 import com.google.gerrit.server.cache.CacheModule;
 import com.google.inject.TypeLiteral;
+import java.time.Duration;
 import org.eclipse.jgit.lib.ObjectId;
 
 public class ExternalIdModule extends CacheModule {
@@ -25,21 +26,13 @@ public class ExternalIdModule extends CacheModule {
   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.
+        // in the latest value. However, due to a race condition, it is possible for different
-        // This can lead to extra cache loads in case of the following race:
+        // threads to observe different values of the meta ref, and hence request different keys
-        // 1. thread 1 reads the notes ref at revision A
+        // from the cache. Extend the cache size by 1 to cover this case, but expire the extra
-        // 2. thread 2 updates the notes ref to revision B and stores the derived value
+        // object after a short period of time, since it may be a potentially large amount of
-        //    for B in the cache
+        // memory.
-        // 3. thread 1 attempts to read the data for revision A from the cache, and misses
+        .maximumWeight(2)
-        // 4. later threads attempt to read at B
+        .expireFromMemoryAfterAccess(Duration.ofMinutes(1))
-        // 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);