Replace ProjectCacheClock with refreshAfterWrite

This commit replaces a home-grown piece of infrastructure with
a standard component that has the same behavior.

ProjectCacheClock was used to detect values in ProjectCache that
are old enough that they might be out of date. It would then
schedule a task on a background thread to check the value.

Both Guava Caches and Caffeine Caches offer this as standard
functionality: refreshAfterWrite.

This is even better than what we had with ProjectCacheClock
because it does the actual refreshing (= reloading the value)
on the background thread.

In a prior commit, we have implemented this option in our own
Cache config framework and the H2 persistent store.

This commit makes use of that prework in the ProjectCache. We
set the default refreshAfterWrite to 15 minutes and the
expireAfterWrite - the duration after which a value is not
read anymore - to 1 hour. Both are configurable. It is also
possible to disable this behavior entirely.

Similar to ProjectCacheClock, we use a dedicated executor for
refreshing cached values. The executor is generic and not
specific to ProjectCache. It's size is configurable.

Change-Id: I8b163853ae092444edf9ff44bef4f7910f21ba0e

Documentation/config-gerrit.txt

@@ -828,6 +828,49 @@ Default is 128 MiB per cache, except:
 +
 If 0 or negative, disk storage for the cache is disabled.
 
+[[cache.name.expireAfterWrite]]cache.<name>.expireAfterWrite::
++
+Duration after which a cached value will be evicted and not
+read anymore.
++
+Values should use common unit suffixes to express their setting:
++
+* ms, milliseconds
+* s, sec, second, seconds
+* m, min, minute, minutes
+* h, hr, hour, hours
++
+Disabled by default.
+
+[[cache.name.refreshAfterWrite]]cache.<name>.refreshAfterWrite::
++
+Duration after which we asynchronously refresh the cached value.
++
+Values should use common unit suffixes to express their setting:
++
+* ms, milliseconds
+* s, sec, second, seconds
+* m, min, minute, minutes
+* h, hr, hour, hours
++
+This applies only to these caches that support refreshing:
++
+* `"projects"`: Caching project information in-memory. Defaults to 15 minutes.
+
+[[cache.refreshThreadPoolSize]]cache.refreshThreadPoolSize::
++
+Number of threads that are available to refresh cached values that became
+out of date. This applies only to these caches that support refreshing:
++
+* `"projects"`: Caching project information in-memory
++
+Refreshes will only be scheduled on this executor if the values are
+out of sync.
+The check if they are is cheap and always happens on the thread that
+inquires for a cached value.
++
+Defaults to 2.
+
 ==== [[cache_names]]Standard Caches
 
 cache `"accounts"`::
@@ -1125,22 +1168,6 @@ configuration.
 +
 Default is true, enabled.
 
-[[cache.projects.checkFrequency]]cache.projects.checkFrequency::
-+
-How often project configuration should be checked for update from Git.
-Gerrit Code Review caches project access rules and configuration in
-memory, checking the refs/meta/config branch every checkFrequency
-minutes to see if a new revision should be loaded and used for future
-access. Values can be specified using standard time unit abbreviations
-('ms', 'sec', 'min', etc.).
-+
-If set to 0, checks occur every time, which may slow down operations.
-If set to 'disabled' or 'off', no check will ever be done.
-Administrators may force the cache to flush with
-link:cmd-flush-caches.html[gerrit flush-caches].
-+
-Default is 5 minutes.
-
 [[cache.projects.loadOnStartup]]cache.projects.loadOnStartup::
 +
 If the project cache should be loaded during server startup.

Documentation/metrics.txt

@@ -64,6 +64,7 @@ objects needing finalization.
 * `caches/memory_eviction_count`: Memory eviction count.
 * `caches/disk_cached`: Disk entries used by persistent cache.
 * `caches/disk_hit_ratio`: Disk hit ratio for persistent cache.
+* `caches/refresh_count`: The number of refreshes per cache with an indicator if a reload was necessary.
 
 === Change
 

java/com/google/gerrit/acceptance/GerritServer.java

@@ -483,7 +483,6 @@ public class GerritServer implements AutoCloseable {
     cfg.setString("gerrit", null, "basePath", "git");
     cfg.setBoolean("sendemail", null, "enable", true);
     cfg.setInt("sendemail", null, "threadPoolSize", 0);
-    cfg.setInt("cache", "projects", "checkFrequency", 0);
     cfg.setInt("plugins", null, "checkFrequency", 0);
 
     cfg.setInt("sshd", null, "threads", 1);

java/com/google/gerrit/server/CacheRefreshExecutor.java

@@ -0,0 +1,28 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server;
+
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+
+import com.google.inject.BindingAnnotation;
+import java.lang.annotation.Retention;
+
+/**
+ * Marker on the global {@link java.util.concurrent.ThreadPoolExecutor} used to refresh outdated
+ * values in caches.
+ */
+@Retention(RUNTIME)
+@BindingAnnotation
+public @interface CacheRefreshExecutor {}

java/com/google/gerrit/server/config/SysExecutorModule.java

@@ -14,7 +14,11 @@
 
 package com.google.gerrit.server.config;
 
+import static com.google.common.util.concurrent.MoreExecutors.newDirectExecutorService;
+
+import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
+import com.google.gerrit.server.CacheRefreshExecutor;
 import com.google.gerrit.server.FanOutExecutor;
 import com.google.gerrit.server.git.WorkQueue;
 import com.google.inject.AbstractModule;
@@ -24,7 +28,7 @@ import java.util.concurrent.ExecutorService;
 import org.eclipse.jgit.lib.Config;
 
 /**
- * Module providing the {@link ReceiveCommitsExecutor}.
+ * Module providing different executors.
  *
  * <p>This module is intended to be installed at the top level when creating a {@code sysInjector}
  * in {@code Daemon} or similar, not nested in another module. This ensures the module can be
@@ -52,7 +56,7 @@ public class SysExecutorModule extends AbstractModule {
       @GerritServerConfig Config config, WorkQueue queues) {
     int poolSize = config.getInt("sendemail", null, "threadPoolSize", 1);
     if (poolSize == 0) {
-      return MoreExecutors.newDirectExecutorService();
+      return newDirectExecutorService();
     }
     return queues.createQueue(poolSize, "SendEmail", true);
   }
@@ -63,8 +67,20 @@ public class SysExecutorModule extends AbstractModule {
   public ExecutorService createFanOutExecutor(@GerritServerConfig Config config, WorkQueue queues) {
     int poolSize = config.getInt("execution", null, "fanOutThreadPoolSize", 25);
     if (poolSize == 0) {
-      return MoreExecutors.newDirectExecutorService();
+      return newDirectExecutorService();
     }
     return queues.createQueue(poolSize, "FanOut");
   }
+
+  @Provides
+  @Singleton
+  @CacheRefreshExecutor
+  public ListeningExecutorService createCacheRefreshExecutor(
+      @GerritServerConfig Config config, WorkQueue queues) {
+    int poolSize = config.getInt("cache", null, "refreshThreadPoolSize", 2);
+    if (poolSize == 0) {
+      return newDirectExecutorService();
+    }
+    return MoreExecutors.listeningDecorator(queues.createQueue(poolSize, "CacheRefresh"));
+  }
 }

java/com/google/gerrit/server/logging/Metadata.java

@@ -107,6 +107,9 @@ public abstract class Metadata {
   // Partial or full computation
   public abstract Optional<Boolean> partial();
 
+  // If a value is still current or not
+  public abstract Optional<Boolean> outdated();
+
   // Path of a metadata file in NoteDb.
   public abstract Optional<String> noteDbFilePath();
 
@@ -305,6 +308,8 @@ public abstract class Metadata {
 
     public abstract Builder partial(boolean partial);
 
+    public abstract Builder outdated(boolean outdated);
+
     public abstract Builder noteDbFilePath(@Nullable String noteDbFilePath);
 
     public abstract Builder noteDbRefName(@Nullable String noteDbRefName);

java/com/google/gerrit/server/project/ProjectCacheClock.java

@@ -1,100 +0,0 @@
-// Copyright (C) 2012 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.project;
-
-import com.google.common.util.concurrent.ThreadFactoryBuilder;
-import com.google.gerrit.extensions.events.LifecycleListener;
-import com.google.gerrit.server.config.ConfigUtil;
-import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.logging.LoggingContextAwareScheduledExecutorService;
-import com.google.inject.Inject;
-import com.google.inject.Singleton;
-import java.util.concurrent.Executors;
-import java.util.concurrent.Future;
-import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicLong;
-import org.eclipse.jgit.lib.Config;
-
-/** Ticks periodically to force refresh events for {@link ProjectCacheImpl}. */
-@Singleton
-public class ProjectCacheClock implements LifecycleListener {
-  private final Config serverConfig;
-
-  private final AtomicLong generation = new AtomicLong();
-
-  private ScheduledExecutorService executor;
-
-  @Inject
-  public ProjectCacheClock(@GerritServerConfig Config serverConfig) {
-    this.serverConfig = serverConfig;
-  }
-
-  @Override
-  public void start() {
-    long checkFrequencyMillis = checkFrequency(serverConfig);
-
-    if (checkFrequencyMillis == Long.MAX_VALUE) {
-      // Start with generation 1 (to avoid magic 0 below).
-      // Do not begin background thread, disabling the clock.
-      generation.set(1);
-    } else if (10 < checkFrequencyMillis) {
-      // Start with generation 1 (to avoid magic 0 below).
-      generation.set(1);
-      executor =
-          new LoggingContextAwareScheduledExecutorService(
-              Executors.newScheduledThreadPool(
-                  1,
-                  new ThreadFactoryBuilder()
-                      .setNameFormat("ProjectCacheClock-%d")
-                      .setDaemon(true)
-                      .setPriority(Thread.MIN_PRIORITY)
-                      .build()));
-      @SuppressWarnings("unused") // Runnable already handles errors
-      Future<?> possiblyIgnoredError =
-          executor.scheduleAtFixedRate(
-              generation::incrementAndGet,
-              checkFrequencyMillis,
-              checkFrequencyMillis,
-              TimeUnit.MILLISECONDS);
-    } else {
-      // Magic generation 0 triggers ProjectState to always
-      // check on each needsRefresh() request we make to it.
-      generation.set(0);
-    }
-  }
-
-  @Override
-  public void stop() {
-    if (executor != null) {
-      executor.shutdown();
-    }
-  }
-
-  long read() {
-    return generation.get();
-  }
-
-  private static long checkFrequency(Config serverConfig) {
-    String freq = serverConfig.getString("cache", "projects", "checkFrequency");
-    if (freq != null && ("disabled".equalsIgnoreCase(freq) || "off".equalsIgnoreCase(freq))) {
-      return Long.MAX_VALUE;
-    }
-    return TimeUnit.MILLISECONDS.convert(
-        ConfigUtil.getTimeUnit(
-            serverConfig, "cache", "projects", "checkFrequency", 5, TimeUnit.MINUTES),
-        TimeUnit.MINUTES);
-  }
-}

java/com/google/gerrit/server/project/ProjectCacheImpl.java

@@ -24,16 +24,23 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Sets;
 import com.google.common.flogger.FluentLogger;
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.entities.AccountGroup;
 import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.RefNames;
 import com.google.gerrit.exceptions.StorageException;
 import com.google.gerrit.index.project.ProjectIndexer;
 import com.google.gerrit.lifecycle.LifecycleModule;
+import com.google.gerrit.metrics.Counter2;
 import com.google.gerrit.metrics.Description;
 import com.google.gerrit.metrics.Description.Units;
+import com.google.gerrit.metrics.Field;
 import com.google.gerrit.metrics.MetricMaker;
 import com.google.gerrit.metrics.Timer0;
+import com.google.gerrit.server.CacheRefreshExecutor;
 import com.google.gerrit.server.cache.CacheModule;
 import com.google.gerrit.server.config.AllProjectsName;
 import com.google.gerrit.server.config.AllUsersName;
@@ -48,6 +55,7 @@ import com.google.inject.Singleton;
 import com.google.inject.TypeLiteral;
 import com.google.inject.name.Named;
 import java.io.IOException;
+import java.time.Duration;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
@@ -55,6 +63,7 @@ import java.util.concurrent.ExecutionException;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
 import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
 
 /** Cache of project information, including access rights. */
@@ -70,7 +79,10 @@ public class ProjectCacheImpl implements ProjectCache {
     return new CacheModule() {
       @Override
       protected void configure() {
-        cache(CACHE_NAME, String.class, ProjectState.class).loader(Loader.class);
+        cache(CACHE_NAME, String.class, ProjectState.class)
+            .loader(Loader.class)
+            .refreshAfterWrite(Duration.ofMinutes(15))
+            .expireAfterWrite(Duration.ofHours(1));
 
         cache(CACHE_LIST, ListKey.class, new TypeLiteral<ImmutableSortedSet<Project.NameKey>>() {})
             .maximumWeight(1)
@@ -84,7 +96,6 @@ public class ProjectCacheImpl implements ProjectCache {
               @Override
               protected void configure() {
                 listener().to(ProjectCacheWarmer.class);
-                listener().to(ProjectCacheClock.class);
               }
             });
       }
@@ -96,7 +107,6 @@ public class ProjectCacheImpl implements ProjectCache {
   private final LoadingCache<String, ProjectState> byName;
   private final LoadingCache<ListKey, ImmutableSortedSet<Project.NameKey>> list;
   private final Lock listLock;
-  private final ProjectCacheClock clock;
   private final Provider<ProjectIndexer> indexer;
   private final Timer0 guessRelevantGroupsLatency;
 
@@ -106,7 +116,6 @@ public class ProjectCacheImpl implements ProjectCache {
       final AllUsersName allUsersName,
       @Named(CACHE_NAME) LoadingCache<String, ProjectState> byName,
       @Named(CACHE_LIST) LoadingCache<ListKey, ImmutableSortedSet<Project.NameKey>> list,
-      ProjectCacheClock clock,
       Provider<ProjectIndexer> indexer,
       MetricMaker metricMaker) {
     this.allProjectsName = allProjectsName;
@@ -114,7 +123,6 @@ public class ProjectCacheImpl implements ProjectCache {
     this.byName = byName;
     this.list = list;
     this.listLock = new ReentrantLock(true /* fair */);
-    this.clock = clock;
     this.indexer = indexer;
 
     this.guessRelevantGroupsLatency =
@@ -142,13 +150,8 @@ public class ProjectCacheImpl implements ProjectCache {
     }
 
     try {
-      ProjectState state = byName.get(projectName.get());
-      if (state != null && state.needsRefresh(clock.read())) {
-        byName.invalidate(projectName.get());
-        state = byName.get(projectName.get());
-      }
-      return Optional.of(state);
-    } catch (Exception e) {
+      return Optional.of(byName.get(projectName.get()));
+    } catch (ExecutionException e) {
       if ((e.getCause() instanceof RepositoryNotFoundException)) {
         logger.atFine().log("Cannot find project %s", projectName.get());
         return Optional.empty();
@@ -245,22 +248,31 @@ public class ProjectCacheImpl implements ProjectCache {
     }
   }
 
+  @Singleton
   static class Loader extends CacheLoader<String, ProjectState> {
     private final ProjectState.Factory projectStateFactory;
     private final GitRepositoryManager mgr;
-    private final ProjectCacheClock clock;
     private final ProjectConfig.Factory projectConfigFactory;
+    private final ListeningExecutorService cacheRefreshExecutor;
+    private final Counter2<String, Boolean> refreshCounter;
 
     @Inject
     Loader(
         ProjectState.Factory psf,
         GitRepositoryManager g,
-        ProjectCacheClock clock,
-        ProjectConfig.Factory projectConfigFactory) {
+        ProjectConfig.Factory projectConfigFactory,
+        @CacheRefreshExecutor ListeningExecutorService cacheRefreshExecutor,
+        MetricMaker metricMaker) {
       projectStateFactory = psf;
       mgr = g;
-      this.clock = clock;
       this.projectConfigFactory = projectConfigFactory;
+      this.cacheRefreshExecutor = cacheRefreshExecutor;
+      refreshCounter =
+          metricMaker.newCounter(
+              "caches/refresh_count",
+              new Description("count").setRate(),
+              Field.ofString("cache", Metadata.Builder::className).build(),
+              Field.ofBoolean("outdated", Metadata.Builder::outdated).build());
     }
 
     @Override
@@ -268,18 +280,37 @@ public class ProjectCacheImpl implements ProjectCache {
       try (TraceTimer timer =
           TraceContext.newTimer(
               "Loading project", Metadata.builder().projectName(projectName).build())) {
-        long now = clock.read();
         Project.NameKey key = Project.nameKey(projectName);
         try (Repository git = mgr.openRepository(key)) {
           ProjectConfig cfg = projectConfigFactory.create(key);
           cfg.load(key, git);
-
-          ProjectState state = projectStateFactory.create(cfg);
-          state.initLastCheck(now);
-          return state;
+          return projectStateFactory.create(cfg);
         }
       }
     }
+
+    @Override
+    public ListenableFuture<ProjectState> reload(String projectName, ProjectState oldState)
+        throws Exception {
+      try (TraceTimer timer =
+          TraceContext.newTimer(
+              "Reload project", Metadata.builder().projectName(projectName).build())) {
+        Project.NameKey key = Project.nameKey(projectName);
+        try (Repository git = mgr.openRepository(key)) {
+          Ref configRef = git.exactRef(RefNames.REFS_CONFIG);
+          if (configRef != null
+              && configRef.getObjectId().equals(oldState.getConfig().getRevision())) {
+            refreshCounter.increment(CACHE_NAME, false);
+            return Futures.immediateFuture(oldState);
+          }
+        }
+
+        // Repository is not thread safe, so we have to open it on the thread that does the loading.
+        // Just invoke the loader on the other thread.
+        refreshCounter.increment(CACHE_NAME, true);
+        return cacheRefreshExecutor.submit(() -> load(projectName));
+      }
+    }
   }
 
   static class ListKey {

java/com/google/gerrit/server/project/ProjectState.java

@@ -32,7 +32,6 @@ import com.google.gerrit.entities.AccountGroup;
 import com.google.gerrit.entities.BooleanProjectConfig;
 import com.google.gerrit.entities.BranchNameKey;
 import com.google.gerrit.entities.Project;
-import com.google.gerrit.entities.RefNames;
 import com.google.gerrit.extensions.api.projects.CommentLinkInfo;
 import com.google.gerrit.extensions.client.SubmitType;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -59,7 +58,6 @@ import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
 
 /**
@@ -86,9 +84,6 @@ public class ProjectState {
   private final long globalMaxObjectSizeLimit;
   private final boolean inheritProjectMaxObjectSizeLimit;
 
-  /** Last system time the configuration's revision was examined. */
-  private volatile long lastCheckGeneration;
-
   /** Local access sections, wrapped in SectionMatchers for faster evaluation. */
   private volatile List<SectionMatcher> localAccessSections;
 
@@ -140,33 +135,6 @@ public class ProjectState {
     }
   }
 
-  void initLastCheck(long generation) {
-    lastCheckGeneration = generation;
-  }
-
-  boolean needsRefresh(long generation) {
-    if (generation <= 0) {
-      return isRevisionOutOfDate();
-    }
-    if (lastCheckGeneration != generation) {
-      lastCheckGeneration = generation;
-      return isRevisionOutOfDate();
-    }
-    return false;
-  }
-
-  private boolean isRevisionOutOfDate() {
-    try (Repository git = gitMgr.openRepository(getNameKey())) {
-      Ref ref = git.getRefDatabase().exactRef(RefNames.REFS_CONFIG);
-      if (ref == null || ref.getObjectId() == null) {
-        return true;
-      }
-      return !ref.getObjectId().equals(config.getRevision());
-    } catch (IOException gone) {
-      return true;
-    }
-  }
-
   /**
    * @return cached computation of all global capabilities. This should only be invoked on the state
    *     from {@link ProjectCache#getAllProjects()}. Null on any other project.

java/com/google/gerrit/testing/InMemoryModule.java

@@ -15,10 +15,11 @@
 package com.google.gerrit.testing;
 
 import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.util.concurrent.MoreExecutors.newDirectExecutorService;
 import static com.google.inject.Scopes.SINGLETON;
 
 import com.google.common.base.Strings;
-import com.google.common.util.concurrent.MoreExecutors;
+import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
 import com.google.gerrit.acceptance.testsuite.project.ProjectOperationsImpl;
 import com.google.gerrit.extensions.client.AuthType;
@@ -30,6 +31,7 @@ import com.google.gerrit.index.SchemaDefinitions;
 import com.google.gerrit.index.project.ProjectSchemaDefinitions;
 import com.google.gerrit.metrics.DisabledMetricMaker;
 import com.google.gerrit.metrics.MetricMaker;
+import com.google.gerrit.server.CacheRefreshExecutor;
 import com.google.gerrit.server.FanOutExecutor;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.GerritPersonIdentProvider;
@@ -209,7 +211,7 @@ public class InMemoryModule extends FactoryModule {
           @Singleton
           @DiffExecutor
           public ExecutorService createDiffExecutor() {
-            return MoreExecutors.newDirectExecutorService();
+            return newDirectExecutorService();
           }
         });
     install(new DefaultMemoryCacheModule());
@@ -277,7 +279,7 @@ public class InMemoryModule extends FactoryModule {
   @Singleton
   @SendEmailExecutor
   public ExecutorService createSendEmailExecutor() {
-    return MoreExecutors.newDirectExecutorService();
+    return newDirectExecutorService();
   }
 
   @Provides
@@ -287,6 +289,13 @@ public class InMemoryModule extends FactoryModule {
     return queues.createQueue(2, "FanOut");
   }
 
+  @Provides
+  @Singleton
+  @CacheRefreshExecutor
+  public ListeningExecutorService createCacheRefreshExecutor() {
+    return newDirectExecutorService();
+  }
+
   private Module luceneIndexModule() {
     return indexModule("com.google.gerrit.lucene.LuceneIndexModule");
   }