Merge changes from topic "expire-after-access"

* changes:
  Use expireAfterAccess with a larger cache size for ExternalIdCache
  Use CacheModule for the external ID cache again
  Support expiring from in-memory caches after last access
  Convert cache expireAfterWrite to nullable Duration

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) {
-    this.extIdsByAccount =
-        CacheBuilder.newBuilder()
-            // The cached data is potentially pretty large and we are always only interested
-            // 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));
+  ExternalIdCacheImpl(
+      @Named(CACHE_NAME) LoadingCache<ObjectId, AllExternalIds> extIdsByAccount,
+      ExternalIdReader externalIdReader) {
+    this.extIdsByAccount = extIdsByAccount;
     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,27 @@
 
 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 java.time.Duration;
+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. However, due to a race condition, it is possible for different
+        // threads to observe different values of the meta ref, and hence request different keys
+        // from the cache. Extend the cache size by 1 to cover this case, but expire the extra
+        // object after a short period of time, since it may be a potentially large amount of
+        // memory.
+        .maximumWeight(2)
+        .expireFromMemoryAfterAccess(Duration.ofMinutes(1))
+        .loader(Loader.class);
+
     bind(ExternalIdCacheImpl.class);
     bind(ExternalIdCache.class).to(ExternalIdCacheImpl.class);
   }

java/com/google/gerrit/server/auth/ldap/LdapModule.java

@@ -14,8 +14,6 @@
 
 package com.google.gerrit.server.auth.ldap;
 
-import static java.util.concurrent.TimeUnit.HOURS;
-
 import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.reviewdb.client.Account;
@@ -25,6 +23,7 @@ import com.google.gerrit.server.account.Realm;
 import com.google.gerrit.server.cache.CacheModule;
 import com.google.inject.Scopes;
 import com.google.inject.TypeLiteral;
+import java.time.Duration;
 import java.util.Optional;
 import java.util.Set;
 
@@ -37,18 +36,18 @@ public class LdapModule extends CacheModule {
   @Override
   protected void configure() {
     cache(GROUP_CACHE, String.class, new TypeLiteral<Set<AccountGroup.UUID>>() {})
-        .expireAfterWrite(1, HOURS)
+        .expireAfterWrite(Duration.ofHours(1))
         .loader(LdapRealm.MemberLoader.class);
 
     cache(USERNAME_CACHE, String.class, new TypeLiteral<Optional<Account.Id>>() {})
         .loader(LdapRealm.UserLoader.class);
 
     cache(GROUP_EXIST_CACHE, String.class, new TypeLiteral<Boolean>() {})
-        .expireAfterWrite(1, HOURS)
+        .expireAfterWrite(Duration.ofHours(1))
         .loader(LdapRealm.ExistenceLoader.class);
 
     cache(PARENT_GROUPS_CACHE, String.class, new TypeLiteral<ImmutableSet<String>>() {})
-        .expireAfterWrite(1, HOURS);
+        .expireAfterWrite(Duration.ofHours(1));
 
     bind(Helper.class);
     bind(Realm.class).to(LdapRealm.class).in(Scopes.SINGLETON);

java/com/google/gerrit/server/cache/CacheBinding.java

@@ -16,15 +16,18 @@ package com.google.gerrit.server.cache;
 
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.Weigher;
-import java.util.concurrent.TimeUnit;
+import java.time.Duration;
 
 /** Configure a cache declared within a {@link CacheModule} instance. */
 public interface CacheBinding<K, V> {
   /** Set the total size of the cache. */
   CacheBinding<K, V> maximumWeight(long weight);
 
-  /** Set the time an element lives before being expired. */
-  CacheBinding<K, V> expireAfterWrite(long duration, TimeUnit durationUnits);
+  /** Set the time an element lives after last write before being expired. */
+  CacheBinding<K, V> expireAfterWrite(Duration duration);
+
+  /** Set the time an element lives after last access before being expired. */
+  CacheBinding<K, V> expireFromMemoryAfterAccess(Duration duration);
 
   /** Populate the cache with items from the CacheLoader. */
   CacheBinding<K, V> loader(Class<? extends CacheLoader<K, V>> clazz);

java/com/google/gerrit/server/cache/CacheDef.java

@@ -18,7 +18,7 @@ import com.google.common.cache.CacheLoader;
 import com.google.common.cache.Weigher;
 import com.google.gerrit.common.Nullable;
 import com.google.inject.TypeLiteral;
-import java.util.concurrent.TimeUnit;
+import java.time.Duration;
 
 public interface CacheDef<K, V> {
   /**
@@ -45,7 +45,10 @@ public interface CacheDef<K, V> {
   long maximumWeight();
 
   @Nullable
-  Long expireAfterWrite(TimeUnit unit);
+  Duration expireAfterWrite();
+
+  @Nullable
+  Duration expireFromMemoryAfterAccess();
 
   @Nullable
   Weigher<K, V> weigher();

java/com/google/gerrit/server/cache/CacheProvider.java

@@ -16,7 +16,6 @@ package com.google.gerrit.server.cache;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
-import static java.util.concurrent.TimeUnit.SECONDS;
 
 import com.google.common.base.Strings;
 import com.google.common.cache.Cache;
@@ -27,7 +26,7 @@ import com.google.gerrit.extensions.annotations.PluginName;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.TypeLiteral;
-import java.util.concurrent.TimeUnit;
+import java.time.Duration;
 
 class CacheProvider<K, V> implements Provider<Cache<K, V>>, CacheBinding<K, V>, CacheDef<K, V> {
   private final CacheModule module;
@@ -36,7 +35,8 @@ class CacheProvider<K, V> implements Provider<Cache<K, V>>, CacheBinding<K, V>,
   private final TypeLiteral<V> valType;
   private String configKey;
   private long maximumWeight;
-  private Long expireAfterWrite;
+  private Duration expireAfterWrite;
+  private Duration expireFromMemoryAfterAccess;
   private Provider<CacheLoader<K, V>> loader;
   private Provider<Weigher<K, V>> weigher;
 
@@ -69,9 +69,16 @@ class CacheProvider<K, V> implements Provider<Cache<K, V>>, CacheBinding<K, V>,
   }
 
   @Override
-  public CacheBinding<K, V> expireAfterWrite(long duration, TimeUnit unit) {
+  public CacheBinding<K, V> expireAfterWrite(Duration duration) {
     checkNotFrozen();
-    expireAfterWrite = SECONDS.convert(duration, unit);
+    expireAfterWrite = duration;
+    return this;
+  }
+
+  @Override
+  public CacheBinding<K, V> expireFromMemoryAfterAccess(Duration duration) {
+    checkNotFrozen();
+    expireFromMemoryAfterAccess = duration;
     return this;
   }
 
@@ -126,8 +133,14 @@ class CacheProvider<K, V> implements Provider<Cache<K, V>>, CacheBinding<K, V>,
 
   @Override
   @Nullable
-  public Long expireAfterWrite(TimeUnit unit) {
-    return expireAfterWrite != null ? unit.convert(expireAfterWrite, SECONDS) : null;
+  public Duration expireAfterWrite() {
+    return expireAfterWrite;
+  }
+
+  @Override
+  @Nullable
+  public Duration expireFromMemoryAfterAccess() {
+    return expireFromMemoryAfterAccess;
   }
 
   @Override

java/com/google/gerrit/server/cache/PersistentCacheBinding.java

@@ -16,7 +16,7 @@ package com.google.gerrit.server.cache;
 
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.Weigher;
-import java.util.concurrent.TimeUnit;
+import java.time.Duration;
 
 /** Configure a persistent cache declared within a {@link CacheModule} instance. */
 public interface PersistentCacheBinding<K, V> extends CacheBinding<K, V> {
@@ -24,7 +24,7 @@ public interface PersistentCacheBinding<K, V> extends CacheBinding<K, V> {
   PersistentCacheBinding<K, V> maximumWeight(long weight);
 
   @Override
-  PersistentCacheBinding<K, V> expireAfterWrite(long duration, TimeUnit durationUnits);
+  PersistentCacheBinding<K, V> expireAfterWrite(Duration duration);
 
   @Override
   PersistentCacheBinding<K, V> loader(Class<? extends CacheLoader<K, V>> clazz);

java/com/google/gerrit/server/cache/PersistentCacheProvider.java

@@ -24,7 +24,7 @@ import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.TypeLiteral;
 import java.io.Serializable;
-import java.util.concurrent.TimeUnit;
+import java.time.Duration;
 
 class PersistentCacheProvider<K, V> extends CacheProvider<K, V>
     implements Provider<Cache<K, V>>, PersistentCacheBinding<K, V>, PersistentCacheDef<K, V> {
@@ -53,8 +53,8 @@ class PersistentCacheProvider<K, V> extends CacheProvider<K, V>
   }
 
   @Override
-  public PersistentCacheBinding<K, V> expireAfterWrite(long duration, TimeUnit durationUnits) {
-    return (PersistentCacheBinding<K, V>) super.expireAfterWrite(duration, durationUnits);
+  public PersistentCacheBinding<K, V> expireAfterWrite(Duration duration) {
+    return (PersistentCacheBinding<K, V>) super.expireAfterWrite(duration);
   }
 
   @Override

java/com/google/gerrit/server/cache/h2/BUILD

@@ -3,6 +3,7 @@ java_library(
     srcs = glob(["**/*.java"]),
     visibility = ["//visibility:public"],
     deps = [
+        "//java/com/google/gerrit/common:annotations",
         "//java/com/google/gerrit/common:server",
         "//java/com/google/gerrit/extensions:api",
         "//java/com/google/gerrit/lifecycle",

java/com/google/gerrit/server/cache/h2/H2CacheDefProxy.java

@@ -16,11 +16,12 @@ package com.google.gerrit.server.cache.h2;
 
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.Weigher;
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.server.cache.CacheSerializer;
 import com.google.gerrit.server.cache.PersistentCacheDef;
 import com.google.gerrit.server.cache.h2.H2CacheImpl.ValueHolder;
 import com.google.inject.TypeLiteral;
-import java.util.concurrent.TimeUnit;
+import java.time.Duration;
 
 class H2CacheDefProxy<K, V> implements PersistentCacheDef<K, V> {
   private final PersistentCacheDef<K, V> source;
@@ -30,8 +31,15 @@ class H2CacheDefProxy<K, V> implements PersistentCacheDef<K, V> {
   }
 
   @Override
-  public Long expireAfterWrite(TimeUnit unit) {
-    return source.expireAfterWrite(unit);
+  @Nullable
+  public Duration expireAfterWrite() {
+    return source.expireAfterWrite();
+  }
+
+  @Override
+  @Nullable
+  public Duration expireFromMemoryAfterAccess() {
+    return source.expireFromMemoryAfterAccess();
   }
 
   @SuppressWarnings("unchecked")

java/com/google/gerrit/server/cache/h2/H2CacheFactory.java

@@ -215,7 +215,6 @@ class H2CacheFactory implements PersistentCacheFactory, LifecycleListener {
     if (h2AutoServer) {
       url.append(";AUTO_SERVER=TRUE");
     }
-    Long expireAfterWrite = def.expireAfterWrite(TimeUnit.SECONDS);
     return new SqlStore<>(
         url.toString(),
         def.keyType(),
@@ -223,6 +222,6 @@ class H2CacheFactory implements PersistentCacheFactory, LifecycleListener {
         def.valueSerializer(),
         def.version(),
         maxSize,
-        expireAfterWrite == null ? 0 : expireAfterWrite.longValue());
+        def.expireAfterWrite());
   }
 }

java/com/google/gerrit/server/cache/h2/H2CacheImpl.java

@@ -23,6 +23,7 @@ import com.google.common.cache.LoadingCache;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.flogger.FluentLogger;
 import com.google.common.hash.BloomFilter;
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.server.cache.CacheSerializer;
 import com.google.gerrit.server.cache.PersistentCache;
@@ -35,6 +36,7 @@ import java.sql.ResultSet;
 import java.sql.SQLException;
 import java.sql.Statement;
 import java.sql.Timestamp;
+import java.time.Duration;
 import java.util.Calendar;
 import java.util.Map;
 import java.util.concurrent.ArrayBlockingQueue;
@@ -254,7 +256,7 @@ public class H2CacheImpl<K, V> extends AbstractLoadingCache<K, V> implements Per
     private final CacheSerializer<V> valueSerializer;
     private final int version;
     private final long maxSize;
-    private final long expireAfterWrite;
+    @Nullable private final Duration expireAfterWrite;
     private final BlockingQueue<SqlHandle> handles;
     private final AtomicLong hitCount = new AtomicLong();
     private final AtomicLong missCount = new AtomicLong();
@@ -268,7 +270,7 @@ public class H2CacheImpl<K, V> extends AbstractLoadingCache<K, V> implements Per
         CacheSerializer<V> valueSerializer,
         int version,
         long maxSize,
-        long expireAfterWrite) {
+        @Nullable Duration expireAfterWrite) {
       this.url = jdbcUrl;
       this.keyType = createKeyType(keyType, keySerializer);
       this.valueSerializer = valueSerializer;
@@ -421,11 +423,11 @@ public class H2CacheImpl<K, V> extends AbstractLoadingCache<K, V> implements Per
     }
 
     private boolean expired(Timestamp created) {
-      if (expireAfterWrite == 0) {
+      if (expireAfterWrite == null) {
         return false;
       }
-      long age = TimeUtil.nowMs() - created.getTime();
-      return 1000 * expireAfterWrite < age;
+      Duration age = Duration.between(created.toInstant(), TimeUtil.now());
+      return age.compareTo(expireAfterWrite) > 0;
     }
 
     private void touch(SqlHandle c, K key) throws IOException, SQLException {

java/com/google/gerrit/server/cache/mem/BUILD

@@ -3,6 +3,7 @@ java_library(
     srcs = glob(["*.java"]),
     visibility = ["//visibility:public"],
     deps = [
+        "//java/com/google/gerrit/common:annotations",
         "//java/com/google/gerrit/extensions:api",
         "//java/com/google/gerrit/server",
         "//lib:guava",

java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java

@@ -14,19 +14,23 @@
 
 package com.google.gerrit.server.cache.mem;
 
+import static java.util.concurrent.TimeUnit.NANOSECONDS;
+import static java.util.concurrent.TimeUnit.SECONDS;
+
 import com.google.common.base.Strings;
 import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
 import com.google.common.cache.Weigher;
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.server.cache.CacheDef;
 import com.google.gerrit.server.cache.ForwardingRemovalListener;
 import com.google.gerrit.server.cache.MemoryCacheFactory;
 import com.google.gerrit.server.config.ConfigUtil;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.inject.Inject;
-import java.util.concurrent.TimeUnit;
+import java.time.Duration;
 import org.eclipse.jgit.lib.Config;
 
 class DefaultMemoryCacheFactory implements MemoryCacheFactory {
@@ -66,19 +70,38 @@ class DefaultMemoryCacheFactory implements MemoryCacheFactory {
     }
     builder.weigher(weigher);
 
-    Long age = def.expireAfterWrite(TimeUnit.SECONDS);
+    Duration expireAfterWrite = def.expireAfterWrite();
     if (has(def.configKey(), "maxAge")) {
       builder.expireAfterWrite(
           ConfigUtil.getTimeUnit(
-              cfg, "cache", def.configKey(), "maxAge", age != null ? age : 0, TimeUnit.SECONDS),
-          TimeUnit.SECONDS);
-    } else if (age != null) {
-      builder.expireAfterWrite(age, TimeUnit.SECONDS);
+              cfg, "cache", def.configKey(), "maxAge", toSeconds(expireAfterWrite), SECONDS),
+          SECONDS);
+    } else if (expireAfterWrite != null) {
+      builder.expireAfterWrite(expireAfterWrite.toNanos(), NANOSECONDS);
+    }
+
+    Duration expireAfterAccess = def.expireFromMemoryAfterAccess();
+    if (has(def.configKey(), "expireFromMemoryAfterAccess")) {
+      builder.expireAfterAccess(
+          ConfigUtil.getTimeUnit(
+              cfg,
+              "cache",
+              def.configKey(),
+              "expireFromMemoryAfterAccess",
+              toSeconds(expireAfterAccess),
+              SECONDS),
+          SECONDS);
+    } else if (expireAfterAccess != null) {
+      builder.expireAfterAccess(expireAfterAccess.toNanos(), NANOSECONDS);
     }
 
     return builder;
   }
 
+  private static long toSeconds(@Nullable Duration duration) {
+    return duration != null ? duration.getSeconds() : 0;
+  }
+
   private boolean has(String name, String var) {
     return !Strings.isNullOrEmpty(cfg.getString("cache", name, var));
   }