Rework SearchingChangeCacheImpl

When filtering refs, we previously had two mechanisms for getting change
information somewhat efficiently. If the instance is running as a
master, we used SearchingChangeCacheImpl to get all change information
from the change index. We parsed this and used if for permission
filtering.

In slaves, we scanned the repo and parsed the changes from disk. There
are two reasons for this: (1) we don't have a change index
available in slaves. (2) There was no cross-machine eviction.

In the master, SearchingChangeCacheImpl was disabled by default, so
without manual configuration, we issued one index call for each
list-refs call.

In addition, we accepted potential staleness of the index. If a change
was moved (= the target branch changed) or marked as private and we
missed the change index update, it would still be available in Git.

This commit reworks the way how we retrieve and store change data for
ref filtering:

We remove SearchingChangeCacheImpl and replace it with ChangeRefCache.
ChangeRefCache has a different caching mechanism. The key contains
project, changeId and the SHA1 of the meta ref. This makes it so that we
can spare any custom eviction logic and have the Guava cache do it's
internal eviction purely based on cache size.

This makes the cache suitable for Gerrit slaves as well.

In addition, we adapt the way how we load change information: If the
change index is available, we bootstrap the cache once per JVM for each
project using the index. All subsequent updates are done incrementally
using the persisted ChangeNotesCache. This drastically cuts down on the
number of index calls we do (one per project per instance vs one per
request) while not sacrificing on the benefits. Due to the (comparably)
low number of change updates per instance and the (comparably) high number
of list ref calls, we will do a very small amount of incremental updates
of the cache for any given list ref call.

For slaves that don't have a change index available, we don't do any
bootstrapping for now and will use the ChangeNotesCache as requests come
in. In case this is too slow, we can easily bootstrap the cache in a
slave using a lifecycle listener. This will be added in the future if
there is a need.

Why do we need this cache at all now that we have a ChangeNotesCache?
To efficiently filter change refs, all (or nearly all) of the
information needs to be in-memory. The ChangeNotesCache is rather large
and on googlesource.com we can't hold all of it in memory.
ChangeRefCache has a small enough footprint that we can. This might
very well be true for other Gerrit instances as well. In case it is not,
administrators can choose to disable ChangeRefCache and have their ref
filtering be backed by ChangeNotesCache.

With this commit, we add integration tests for the new cache and remove
complexity from DefaultRefFilter.

Change-Id: I5eda9d411e97925e3e8b450fe32693a936164f96

Documentation/config-gerrit.txt

@@ -819,18 +819,23 @@ for the client to use as potential delta bases.  Push over smart HTTP
 requires two HTTP requests, and this cache tries to carry state from
 the first request into the second to ensure it can complete.
 
-cache `"changes"`::
+cache `"change_refs"`::
++
+Cache entries are used to compute change ref visibility efficiently. Entries
+contain the minimal required information for this task.
 +
 The size of `memoryLimit` determines the number of projects for which
 all changes will be cached. If the cache is set to 1024, this means all
-changes for up to 1024 projects can be held in the cache.
+at maximum 1024 changes can be held in the cache.
 +
-Default value is 0 (disabled). It is disabled by default due to the fact
-that change updates are not communicated between Gerrit servers. Hence
-this cache should be disabled in an multi-master/multi-slave setup.
+Default value is 10.000.
 +
-The cache should be flushed whenever the database changes table is modified
-outside of Gerrit.
+If the size is set to 0, the cache is disabled. The change index will not
+be used at all. Instead, the change notes cache is used directly.
++
+A good size for this cache is twice the number of changes that the Gerrit
+instance has to allow it to hold all current changes and account for
+growth.
 
 cache `"diff"`::
 +

java/com/google/gerrit/httpd/init/WebAppInitializer.java

@@ -66,9 +66,9 @@ import com.google.gerrit.server.config.SitePath;
 import com.google.gerrit.server.config.SysExecutorModule;
 import com.google.gerrit.server.events.EventBroker;
 import com.google.gerrit.server.events.StreamEventsApiListener;
+import com.google.gerrit.server.git.ChangeRefCache;
 import com.google.gerrit.server.git.GarbageCollectionModule;
 import com.google.gerrit.server.git.GitRepositoryManagerModule;
-import com.google.gerrit.server.git.SearchingChangeCacheImpl;
 import com.google.gerrit.server.git.WorkQueue;
 import com.google.gerrit.server.index.IndexModule;
 import com.google.gerrit.server.index.IndexModule.IndexType;
@@ -288,7 +288,7 @@ public class WebAppInitializer extends GuiceServletContextListener implements Fi
     modules.add(cfgInjector.getInstance(GerritGlobalModule.class));
     modules.add(new GerritApiModule());
     modules.add(new PluginApiModule());
-    modules.add(new SearchingChangeCacheImpl.Module());
+    modules.add(new ChangeRefCache.Module());
     modules.add(new InternalAccountDirectory.Module());
     modules.add(new DefaultPermissionBackendModule());
     modules.add(new DefaultMemoryCacheModule());

java/com/google/gerrit/pgm/Daemon.java

@@ -73,8 +73,8 @@ import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.SysExecutorModule;
 import com.google.gerrit.server.events.EventBroker;
 import com.google.gerrit.server.events.StreamEventsApiListener;
+import com.google.gerrit.server.git.ChangeRefCache;
 import com.google.gerrit.server.git.GarbageCollectionModule;
-import com.google.gerrit.server.git.SearchingChangeCacheImpl;
 import com.google.gerrit.server.git.WorkQueue;
 import com.google.gerrit.server.group.PeriodicGroupIndexer;
 import com.google.gerrit.server.index.IndexModule;
@@ -393,7 +393,7 @@ public class Daemon extends SiteProgram {
     modules.add(new GerritApiModule());
     modules.add(new PluginApiModule());
 
-    modules.add(new SearchingChangeCacheImpl.Module(slave));
+    modules.add(new ChangeRefCache.Module());
     modules.add(new InternalAccountDirectory.Module());
     modules.add(new DefaultPermissionBackendModule());
     modules.add(new DefaultMemoryCacheModule());

java/com/google/gerrit/pgm/util/BatchProgramModule.java

@@ -56,8 +56,8 @@ import com.google.gerrit.server.config.SysExecutorModule;
 import com.google.gerrit.server.extensions.events.EventUtil;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.extensions.events.RevisionCreated;
+import com.google.gerrit.server.git.ChangeRefCache;
 import com.google.gerrit.server.git.MergeUtil;
-import com.google.gerrit.server.git.SearchingChangeCacheImpl;
 import com.google.gerrit.server.git.TagCache;
 import com.google.gerrit.server.mail.send.ReplacePatchSetSender;
 import com.google.gerrit.server.notedb.NoteDbModule;
@@ -133,7 +133,7 @@ public class BatchProgramModule extends FactoryModule {
 
     // As Reindex is a batch program, don't assume the index is available for
     // the change cache.
-    bind(SearchingChangeCacheImpl.class).toProvider(Providers.of(null));
+    bind(ChangeRefCache.class).toProvider(Providers.of(null));
 
     bind(new TypeLiteral<ImmutableSet<GroupReference>>() {})
         .annotatedWith(AdministrateServerGroups.class)

java/com/google/gerrit/server/git/ChangeRefCache.java

@@ -0,0 +1,226 @@
+// Copyright (C) 2019 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.git;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.index.RefState;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.ReviewerSet;
+import com.google.gerrit.server.cache.CacheModule;
+import com.google.gerrit.server.config.GerritOptions;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.index.change.ChangeField;
+import com.google.gerrit.server.logging.TraceContext;
+import com.google.gerrit.server.logging.TraceContext.TraceTimer;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.InternalChangeQuery;
+import com.google.gerrit.server.util.ManualRequestContext;
+import com.google.gerrit.server.util.OneOffRequestContext;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+import com.google.inject.TypeLiteral;
+import com.google.inject.name.Named;
+import java.util.List;
+import java.util.NoSuchElementException;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.CopyOnWriteArraySet;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.ObjectId;
+
+/**
+ * Cache for the minimal information per change that we need to compute visibility. Used for ref
+ * filtering.
+ *
+ * <p>This class is thread safe.
+ */
+@Singleton
+public class ChangeRefCache {
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+  static final String ID_CACHE = "change_refs";
+
+  public static class Module extends CacheModule {
+    @Override
+    protected void configure() {
+      cache(ID_CACHE, Key.class, new TypeLiteral<CachedChange>() {})
+          .maximumWeight(10000)
+          .loader(Loader.class);
+
+      bind(ChangeRefCache.class);
+    }
+  }
+
+  @AutoValue
+  abstract static class Key {
+    abstract Project.NameKey project();
+
+    abstract Change.Id changeId();
+
+    abstract ObjectId metaId();
+  }
+
+  @AutoValue
+  abstract static class CachedChange {
+    // Subset of fields in ChangeData, specifically fields needed to serve
+    // VisibleRefFilter without touching the database. More can be added as
+    // necessary.
+    abstract Change change();
+
+    @Nullable
+    abstract ReviewerSet reviewers();
+  }
+
+  private final LoadingCache<Key, CachedChange> cache;
+  private final ChangeData.Factory changeDataFactory;
+  private final OneOffRequestContext requestContext;
+  private final Provider<InternalChangeQuery> queryProvider;
+  private final GerritOptions gerritOptions;
+  private final Config gerritConfig;
+  private final Set<Project.NameKey> bootstrappedProjects;
+
+  @Inject
+  ChangeRefCache(
+      @Named(ID_CACHE) LoadingCache<Key, CachedChange> cache,
+      ChangeData.Factory changeDataFactory,
+      OneOffRequestContext requestContext,
+      Provider<InternalChangeQuery> queryProvider,
+      GerritOptions gerritOptions,
+      @GerritServerConfig Config gerritConfig) {
+    this.cache = cache;
+    this.changeDataFactory = changeDataFactory;
+    this.requestContext = requestContext;
+    this.queryProvider = queryProvider;
+    this.gerritOptions = gerritOptions;
+    this.gerritConfig = gerritConfig;
+    // Uses a CopyOnWriteArraySet internally to keep track of projects that are already
+    // bootstrapped. This is efficient because we read from the set on every call to this method to
+    // check if bootstrapping is required. Writes occur only if we bootstrapped, so once per
+    // project.
+    this.bootstrappedProjects = new CopyOnWriteArraySet<>();
+  }
+
+  /**
+   * Read changes from the cache.
+   *
+   * <p>Returned changes only include the {@code Change} object (with id, branch) and the reviewers.
+   * There is no guarantee that additional fields are populated, although they can be.
+   *
+   * @param project project to read.
+   * @param changeId change ID to read
+   * @param metaId object ID of the meta branch to read. This is only used to ensure consistency. It
+   *     does not allow for reading non-current meta versions.
+   * @return change data
+   * @throws IllegalArgumentException in case no change is found
+   */
+  public ChangeData getChangeData(Project.NameKey project, Change.Id changeId, ObjectId metaId) {
+    Key key = new AutoValue_ChangeRefCache_Key(project, changeId, metaId);
+    CachedChange cached = cache.getUnchecked(key);
+    if (cached == null) {
+      throw new IllegalArgumentException("no change found for key " + key);
+    }
+    ChangeData cd = changeDataFactory.create(cached.change());
+    cd.setReviewers(cached.reviewers());
+    return cd;
+  }
+
+  /**
+   * This method bootstraps the cache by querying the change index if it hasn't been bootstrapped
+   * before, in which case it is a cheap no-op.
+   *
+   * @param project the project to bootstrap
+   */
+  public void bootstrapIfNecessary(Project.NameKey project) {
+    if (!gerritOptions.enableMasterFeatures()) {
+      // Bootstrapping using the ChangeIndex is only supported on master in a master-slave replica.
+      return;
+    }
+    if (gerritConfig.getInt("cache", ID_CACHE, "memoryLimit", -1) == 0) {
+      // The cache is disabled, don't bother bootstrapping.
+      return;
+    }
+    if (bootstrappedProjects.contains(project)) {
+      // We have bootstrapped for this project before. If the cache is too small, we might have
+      // evicted all entries by now. Don't bother about this though as we don't want to add the
+      // complexity of checking for existing projects, since that might not be authoritative as well
+      // since we could have already evicted the majority of the entries.
+      return;
+    }
+
+    try (TraceTimer ignored =
+            TraceContext.newTimer("bootstrapping ChangeRef cache for project " + project);
+        ManualRequestContext ignored2 = requestContext.open()) {
+      List<ChangeData> cds =
+          queryProvider
+              .get()
+              .setRequestedFields(ChangeField.CHANGE, ChangeField.REVIEWER, ChangeField.REF_STATE)
+              .byProject(project);
+      for (ChangeData cd : cds) {
+        Set<RefState> refStates = RefState.parseStates(cd.getRefStates()).get(project);
+        Optional<RefState> refState =
+            refStates
+                .stream()
+                .filter(r -> r.ref().equals(RefNames.changeMetaRef(cd.getId())))
+                .findAny();
+        if (!refState.isPresent()) {
+          continue;
+        }
+        cache.put(
+            new AutoValue_ChangeRefCache_Key(project, cd.change().getId(), refState.get().id()),
+            new AutoValue_ChangeRefCache_CachedChange(cd.change(), cd.getReviewers()));
+      }
+      // Mark the project as bootstrapped. We could have bootstrapped it multiple times for
+      // simultaneous requests. We accept this in favor of less thread synchronization and
+      // complexity.
+      bootstrappedProjects.add(project);
+    } catch (OrmException e) {
+      logger.atWarning().withCause(e).log(
+          "unable to bootstrap ChangeRef cache for project " + project);
+    }
+  }
+
+  static class Loader extends CacheLoader<Key, CachedChange> {
+    private final ChangeNotes.Factory notesFactory;
+
+    @Inject
+    Loader(ChangeNotes.Factory notesFactory) {
+      this.notesFactory = notesFactory;
+    }
+
+    @Override
+    public CachedChange load(Key key) throws Exception {
+      ChangeNotes notes = notesFactory.create(key.project(), key.changeId());
+      if (notes.getMetaId().equals(key.metaId())) {
+        return new AutoValue_ChangeRefCache_CachedChange(notes.getChange(), notes.getReviewers());
+      }
+      throw new NoSuchElementException("unable to load change");
+    }
+  }
+
+  @VisibleForTesting
+  public void resetBootstrappedProjects() {
+    bootstrappedProjects.clear();
+  }
+}

java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java

@@ -1,162 +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.git;
-
-import com.google.auto.value.AutoValue;
-import com.google.common.cache.CacheLoader;
-import com.google.common.cache.LoadingCache;
-import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.common.Nullable;
-import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
-import com.google.gerrit.extensions.registration.DynamicSet;
-import com.google.gerrit.reviewdb.client.Change;
-import com.google.gerrit.reviewdb.client.Project;
-import com.google.gerrit.reviewdb.client.RefNames;
-import com.google.gerrit.server.ReviewerSet;
-import com.google.gerrit.server.cache.CacheModule;
-import com.google.gerrit.server.index.change.ChangeField;
-import com.google.gerrit.server.logging.TraceContext;
-import com.google.gerrit.server.logging.TraceContext.TraceTimer;
-import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.query.change.InternalChangeQuery;
-import com.google.gerrit.server.util.ManualRequestContext;
-import com.google.gerrit.server.util.OneOffRequestContext;
-import com.google.inject.Inject;
-import com.google.inject.Provider;
-import com.google.inject.Singleton;
-import com.google.inject.TypeLiteral;
-import com.google.inject.name.Named;
-import com.google.inject.util.Providers;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.List;
-import java.util.concurrent.ExecutionException;
-
-@Singleton
-public class SearchingChangeCacheImpl implements GitReferenceUpdatedListener {
-  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
-  static final String ID_CACHE = "changes";
-
-  public static class Module extends CacheModule {
-    private final boolean slave;
-
-    public Module() {
-      this(false);
-    }
-
-    public Module(boolean slave) {
-      this.slave = slave;
-    }
-
-    @Override
-    protected void configure() {
-      if (slave) {
-        bind(SearchingChangeCacheImpl.class).toProvider(Providers.of(null));
-      } else {
-        cache(ID_CACHE, Project.NameKey.class, new TypeLiteral<List<CachedChange>>() {})
-            .maximumWeight(0)
-            .loader(Loader.class);
-
-        bind(SearchingChangeCacheImpl.class);
-        DynamicSet.bind(binder(), GitReferenceUpdatedListener.class)
-            .to(SearchingChangeCacheImpl.class);
-      }
-    }
-  }
-
-  @AutoValue
-  abstract static class CachedChange {
-    // Subset of fields in ChangeData, specifically fields needed to serve
-    // VisibleRefFilter without touching the database. More can be added as
-    // necessary.
-    abstract Change change();
-
-    @Nullable
-    abstract ReviewerSet reviewers();
-  }
-
-  private final LoadingCache<Project.NameKey, List<CachedChange>> cache;
-  private final ChangeData.Factory changeDataFactory;
-
-  @Inject
-  SearchingChangeCacheImpl(
-      @Named(ID_CACHE) LoadingCache<Project.NameKey, List<CachedChange>> cache,
-      ChangeData.Factory changeDataFactory) {
-    this.cache = cache;
-    this.changeDataFactory = changeDataFactory;
-  }
-
-  /**
-   * Read changes for the project from the secondary index.
-   *
-   * <p>Returned changes only include the {@code Change} object (with id, branch) and the reviewers.
-   * Additional stored fields are not loaded from the index.
-   *
-   * @param project project to read.
-   * @return list of known changes; empty if no changes.
-   */
-  public List<ChangeData> getChangeData(Project.NameKey project) {
-    try {
-      List<CachedChange> cached = cache.get(project);
-      List<ChangeData> cds = new ArrayList<>(cached.size());
-      for (CachedChange cc : cached) {
-        ChangeData cd = changeDataFactory.create(cc.change());
-        cd.setReviewers(cc.reviewers());
-        cds.add(cd);
-      }
-      return Collections.unmodifiableList(cds);
-    } catch (ExecutionException e) {
-      logger.atWarning().withCause(e).log("Cannot fetch changes for %s", project);
-      return Collections.emptyList();
-    }
-  }
-
-  @Override
-  public void onGitReferenceUpdated(GitReferenceUpdatedListener.Event event) {
-    if (event.getRefName().startsWith(RefNames.REFS_CHANGES)) {
-      cache.invalidate(new Project.NameKey(event.getProjectName()));
-    }
-  }
-
-  static class Loader extends CacheLoader<Project.NameKey, List<CachedChange>> {
-    private final OneOffRequestContext requestContext;
-    private final Provider<InternalChangeQuery> queryProvider;
-
-    @Inject
-    Loader(OneOffRequestContext requestContext, Provider<InternalChangeQuery> queryProvider) {
-      this.requestContext = requestContext;
-      this.queryProvider = queryProvider;
-    }
-
-    @Override
-    public List<CachedChange> load(Project.NameKey key) throws Exception {
-      try (TraceTimer timer = TraceContext.newTimer("Loading changes of project %s", key);
-          ManualRequestContext ctx = requestContext.open()) {
-        List<ChangeData> cds =
-            queryProvider
-                .get()
-                .setRequestedFields(ChangeField.CHANGE, ChangeField.REVIEWER)
-                .byProject(key);
-        List<CachedChange> result = new ArrayList<>(cds.size());
-        for (ChangeData cd : cds) {
-          result.add(
-              new AutoValue_SearchingChangeCacheImpl_CachedChange(cd.change(), cd.getReviewers()));
-        }
-        return Collections.unmodifiableList(result);
-      }
-    }
-  }
-}

java/com/google/gerrit/server/permissions/DefaultRefFilter.java

@@ -14,7 +14,6 @@
 
 package com.google.gerrit.server.permissions;
 
-import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.gerrit.reviewdb.client.RefNames.REFS_CACHE_AUTOMERGE;
 import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES;
 import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG;
@@ -22,9 +21,7 @@ import static com.google.gerrit.reviewdb.client.RefNames.REFS_USERS_SELF;
 import static java.util.Objects.requireNonNull;
 import static java.util.stream.Collectors.toMap;
 
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Maps;
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.extensions.restapi.AuthException;
@@ -41,12 +38,11 @@ import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.GroupCache;
 import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.git.SearchingChangeCacheImpl;
+import com.google.gerrit.server.git.ChangeRefCache;
 import com.google.gerrit.server.git.TagCache;
 import com.google.gerrit.server.git.TagMatcher;
 import com.google.gerrit.server.group.InternalGroup;
 import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.notedb.ChangeNotes.Factory.ChangeNotesResult;
 import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions;
 import com.google.gerrit.server.project.ProjectState;
 import com.google.gerrit.server.query.change.ChangeData;
@@ -55,10 +51,11 @@ import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
 import java.io.IOException;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.Ref;
@@ -74,7 +71,7 @@ class DefaultRefFilter {
 
   private final TagCache tagCache;
   private final ChangeNotes.Factory changeNotesFactory;
-  @Nullable private final SearchingChangeCacheImpl changeCache;
+  @Nullable private final ChangeRefCache changeCache;
   private final GroupCache groupCache;
   private final PermissionBackend permissionBackend;
   private final ProjectControl projectControl;
@@ -84,14 +81,14 @@ class DefaultRefFilter {
   private final Counter0 fullFilterCount;
   private final Counter0 skipFilterCount;
   private final boolean skipFullRefEvaluationIfAllRefsAreVisible;
-
-  private Map<Change.Id, Branch.NameKey> visibleChanges;
+  private final Map<Change.Id, Branch.NameKey> visibleChanges;
+  private final Set<Change.Id> inVisibleChanges;
 
   @Inject
   DefaultRefFilter(
       TagCache tagCache,
       ChangeNotes.Factory changeNotesFactory,
-      @Nullable SearchingChangeCacheImpl changeCache,
+      @Nullable ChangeRefCache changeCache,
       GroupCache groupCache,
       PermissionBackend permissionBackend,
       @GerritServerConfig Config config,
@@ -121,6 +118,9 @@ class DefaultRefFilter {
                     "Rate of ref filter operations where we skip full evaluation"
                         + " because the user can read all refs")
                 .setRate());
+    // TODO(hiesel): Rework who can see change edits so that we can keep just a single set here.
+    this.visibleChanges = new HashMap<>();
+    this.inVisibleChanges = new HashSet<>();
   }
 
   Map<String, Ref> filter(Map<String, Ref> refs, Repository repo, RefFilterOptions opts)
@@ -161,6 +161,7 @@ class DefaultRefFilter {
 
     Map<String, Ref> result = new HashMap<>();
     List<Ref> deferredTags = new ArrayList<>();
+    changeCache.bootstrapIfNecessary(projectState.getNameKey());
 
     for (Ref ref : refs.values()) {
       String name = ref.getName();
@@ -299,31 +300,55 @@ class DefaultRefFilter {
   }
 
   private boolean visible(Repository repo, Change.Id changeId) throws PermissionBackendException {
-    if (visibleChanges == null) {
-      if (changeCache == null) {
-        visibleChanges = visibleChangesByScan(repo);
-      } else {
-        visibleChanges = visibleChangesBySearch();
+    if (visibleChanges.containsKey(changeId)) {
+      return true;
     }
+    if (inVisibleChanges.contains(changeId)) {
+      return false;
+    }
+    // TODO(hiesel): The project state should be checked once in the beginning an left alone
+    // thereafter.
+    if (!projectState.statePermitsRead()) {
+      return false;
+    }
+
+    Project.NameKey project = projectState.getNameKey();
+    try {
+      ChangeData cd =
+          changeCache.getChangeData(
+              project, changeId, repo.exactRef(RefNames.changeMetaRef(changeId)).getObjectId());
+      ChangeNotes notes = changeNotesFactory.createFromIndexedChange(cd.change());
+      try {
+        permissionBackendForProject.indexedChange(cd, notes).check(ChangePermission.READ);
+        visibleChanges.put(cd.getId(), cd.change().getDest());
+        return true;
+      } catch (AuthException e) {
+        inVisibleChanges.add(changeId);
+        return false;
+      }
+    } catch (OrmException | IOException e) {
+      throw new PermissionBackendException(
+          "Cannot load change " + changeId + " for project " + project + ", assuming not visible",
+          e);
     }
-    return visibleChanges.containsKey(changeId);
   }
 
   private boolean visibleEdit(Repository repo, String name) throws PermissionBackendException {
     Change.Id id = Change.Id.fromEditRefPart(name);
-    // Initialize if it wasn't yet
-    if (visibleChanges == null) {
-      visible(repo, id);
-    }
     if (id == null) {
       return false;
     }
+    if (!visible(repo, id)) {
+      // Can't see the change, so can't see the edit.
+      return false;
+    }
+
     if (user.isIdentifiedUser()
-        && name.startsWith(RefNames.refsEditPrefix(user.asIdentifiedUser().getAccountId()))
-        && visible(repo, id)) {
+        && name.startsWith(RefNames.refsEditPrefix(user.asIdentifiedUser().getAccountId()))) {
+      // Own edit
       return true;
     }
-    if (visibleChanges.containsKey(id)) {
+
     try {
       // Default to READ_PRIVATE_CHANGES as there is no special permission for reading edits.
       permissionBackendForProject
@@ -334,76 +359,6 @@ class DefaultRefFilter {
       return false;
     }
   }
-    return false;
-  }
-
-  private Map<Change.Id, Branch.NameKey> visibleChangesBySearch()
-      throws PermissionBackendException {
-    Project.NameKey project = projectState.getNameKey();
-    try {
-      Map<Change.Id, Branch.NameKey> visibleChanges = new HashMap<>();
-      for (ChangeData cd : changeCache.getChangeData(project)) {
-        ChangeNotes notes = changeNotesFactory.createFromIndexedChange(cd.change());
-        if (!projectState.statePermitsRead()) {
-          continue;
-        }
-        try {
-          permissionBackendForProject.indexedChange(cd, notes).check(ChangePermission.READ);
-          visibleChanges.put(cd.getId(), cd.change().getDest());
-        } catch (AuthException e) {
-          // Do nothing.
-        }
-      }
-      return visibleChanges;
-    } catch (OrmException e) {
-      logger.atSevere().withCause(e).log(
-          "Cannot load changes for project %s, assuming no changes are visible", project);
-      return Collections.emptyMap();
-    }
-  }
-
-  private Map<Change.Id, Branch.NameKey> visibleChangesByScan(Repository repo)
-      throws PermissionBackendException {
-    Project.NameKey p = projectState.getNameKey();
-    ImmutableList<ChangeNotesResult> changes;
-    try {
-      changes = changeNotesFactory.scan(repo, p).collect(toImmutableList());
-    } catch (IOException e) {
-      logger.atSevere().withCause(e).log(
-          "Cannot load changes for project %s, assuming no changes are visible", p);
-      return Collections.emptyMap();
-    }
-
-    Map<Change.Id, Branch.NameKey> result = Maps.newHashMapWithExpectedSize(changes.size());
-    for (ChangeNotesResult notesResult : changes) {
-      ChangeNotes notes = toNotes(notesResult);
-      if (notes != null) {
-        result.put(notes.getChangeId(), notes.getChange().getDest());
-      }
-    }
-    return result;
-  }
-
-  @Nullable
-  private ChangeNotes toNotes(ChangeNotesResult r) throws PermissionBackendException {
-    if (r.error().isPresent()) {
-      logger.atWarning().withCause(r.error().get()).log(
-          "Failed to load change %s in %s", r.id(), projectState.getName());
-      return null;
-    }
-
-    if (!projectState.statePermitsRead()) {
-      return null;
-    }
-
-    try {
-      permissionBackendForProject.change(r.notes()).check(ChangePermission.READ);
-      return r.notes();
-    } catch (AuthException e) {
-      // Skip.
-    }
-    return null;
-  }
 
   private boolean isMetadata(String name) {
     return name.startsWith(REFS_CHANGES) || RefNames.isRefsEdit(name);

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

@@ -58,10 +58,10 @@ import com.google.gerrit.server.config.SendEmailExecutor;
 import com.google.gerrit.server.config.SitePath;
 import com.google.gerrit.server.config.TrackingFooters;
 import com.google.gerrit.server.config.TrackingFootersProvider;
+import com.google.gerrit.server.git.ChangeRefCache;
 import com.google.gerrit.server.git.GarbageCollection;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.PerThreadRequestScope;
-import com.google.gerrit.server.git.SearchingChangeCacheImpl;
 import com.google.gerrit.server.git.WorkQueue;
 import com.google.gerrit.server.index.IndexModule.IndexType;
 import com.google.gerrit.server.index.account.AccountSchemaDefinitions;
@@ -168,7 +168,7 @@ public class InMemoryModule extends FactoryModule {
     factory(PluginUser.Factory.class);
     install(new PluginApiModule());
     install(new DefaultPermissionBackendModule());
-    install(new SearchingChangeCacheImpl.Module());
+    install(new ChangeRefCache.Module());
     factory(GarbageCollection.Factory.class);
     install(new AuditModule());
 

javatests/com/google/gerrit/acceptance/git/ChangeRefCacheIT.java

@@ -0,0 +1,269 @@
+// Copyright (C) 2019 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.acceptance.git;
+
+import static com.google.common.truth.Truth.assertThat;
+import static java.util.stream.Collectors.toMap;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.AcceptanceTestRequestScope;
+import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.git.ChangeRefCache;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions;
+import com.google.gerrit.server.query.change.ChangeData;
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
+import javax.inject.Inject;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * Tests the ChangeRefCache by running ls-remote calls and conditionally disabling the index and
+ * NoteDb. The cache is enabled by default.
+ *
+ * <p>Why are we not just testing ChangeRefCache directly? Our ref filtering code is rather complex
+ * and it is easy to get something wrong there. We want our assumptions about the performance of the
+ * cache to be validated against the entire component rather than just the cache.
+ */
+@NoHttpd
+public class ChangeRefCacheIT extends AbstractDaemonTest {
+
+  @Inject private PermissionBackend permissionBackend;
+  @Inject private ChangeRefCache changeRefCache;
+
+  @Before
+  public void setUp() throws Exception {
+    // We want full ref evaluation so that we hit the cache every time.
+    baseConfig.setBoolean("auth", null, "skipFullRefEvaluationIfAllRefsAreVisible", false);
+  }
+
+  /**
+   * Ensure we use only the change index for getting initial data populated and don't touch NoteDb.
+   */
+  @Test
+  public void useIndexForBootstrapping() throws Exception {
+    ChangeData change = createChange().getChange();
+    // TODO(hiesel) Rework as AutoClosable. Here and below.
+    changeRefCache.resetBootstrappedProjects();
+    AcceptanceTestRequestScope.Context ctx = disableDb();
+    try {
+      assertUploadPackRefs(
+          "HEAD",
+          "refs/heads/master",
+          RefNames.changeMetaRef(change.getId()),
+          change.currentPatchSet().getId().toRefName());
+    } finally {
+      enableDb(ctx);
+    }
+  }
+
+  /**
+   * Ensure we use only the change index for getting initial data populated and don't require any
+   * storage backend after that as long the data didn't change.
+   */
+  @Test
+  public void serveResultsFromCacheAfterInitialBootstrap() throws Exception {
+    ChangeData change = createChange().getChange();
+    changeRefCache.resetBootstrappedProjects();
+    AcceptanceTestRequestScope.Context ctx = disableDb();
+    try {
+      assertUploadPackRefs(
+          "HEAD",
+          "refs/heads/master",
+          RefNames.changeMetaRef(change.getId()),
+          change.currentPatchSet().getId().toRefName());
+    } finally {
+      enableDb(ctx);
+    }
+
+    // No change since our first call, so this time we don't bootstrap or touch NoteDb
+    AcceptanceTestRequestScope.Context ctx2 = disableDb();
+    try {
+      try (AutoCloseable ignored = disableChangeIndex()) {
+        assertUploadPackRefs(
+            "HEAD",
+            "refs/heads/master",
+            RefNames.changeMetaRef(change.getId()),
+            change.currentPatchSet().getId().toRefName());
+      }
+    } finally {
+      enableDb(ctx2);
+    }
+  }
+
+  /**
+   * Ensure we use only the change index for getting initial data populated and NoteDb for reloading
+   * data that changed since.
+   */
+  @Test
+  public void useIndexForBootstrappingAndDbForDeltaReload() throws Exception {
+    ChangeData change1 = createChange().getChange();
+    AcceptanceTestRequestScope.Context ctx = disableDb();
+    // Bootstrap: No NoteDb access as we expect it to use the index.
+    changeRefCache.resetBootstrappedProjects();
+    try {
+      assertUploadPackRefs(
+          "HEAD",
+          "refs/heads/master",
+          RefNames.changeMetaRef(change1.getId()),
+          change1.currentPatchSet().getId().toRefName());
+    } finally {
+      enableDb(ctx);
+    }
+    // Delta reload: No index access as we expect it to use NoteDb.
+    ChangeData change2 = createChange().getChange();
+    try (AutoCloseable ignored = disableChangeIndex()) {
+      assertUploadPackRefs(
+          "HEAD",
+          "refs/heads/master",
+          RefNames.changeMetaRef(change1.getId()),
+          change1.currentPatchSet().getId().toRefName(),
+          RefNames.changeMetaRef(change2.getId()),
+          change2.currentPatchSet().getId().toRefName());
+    }
+  }
+
+  /**
+   * Ensure we use only the change index for getting initial data populated and NoteDb for reloading
+   * data that changed since.
+   */
+  @Test
+  public void useDbForDeltaReloadOnNewPatchSet() throws Exception {
+    ChangeData change1 =
+        pushFactory
+            .create(admin.getIdent(), testRepo, "original subject", "a", "a1")
+            .to("refs/for/master")
+            .getChange();
+
+    AcceptanceTestRequestScope.Context ctx = disableDb();
+    // Bootstrap: No NoteDb access as we expect it to use the index.
+    changeRefCache.resetBootstrappedProjects();
+    try {
+      assertUploadPackRefs(
+          "HEAD",
+          "refs/heads/master",
+          RefNames.changeMetaRef(change1.getId()),
+          change1.currentPatchSet().getId().toRefName());
+    } finally {
+      enableDb(ctx);
+    }
+
+    // Delta reload: No index access as we expect it to use NoteDb.
+    ChangeData change2 =
+        pushFactory
+            .create(
+                admin.getIdent(), testRepo, "subject2", "a", "a2", change1.change().getKey().get())
+            .to("refs/for/master")
+            .getChange();
+    List<PatchSet> patchSets = ImmutableList.copyOf(change2.patchSets());
+    assertThat(patchSets).hasSize(2);
+    try (AutoCloseable ctx2 = disableChangeIndex()) {
+      assertUploadPackRefs(
+          "HEAD",
+          "refs/heads/master",
+          RefNames.changeMetaRef(change1.getId()),
+          patchSets.get(0).getId().toRefName(),
+          patchSets.get(1).getId().toRefName());
+    }
+  }
+
+  /**
+   * Ensure we use only the change index for getting initial data populated and NoteDb for reloading
+   * data that changed since.
+   */
+  @Test
+  public void useDbForIterativeFetchingOnMetadataChange() throws Exception {
+    ChangeData change1 =
+        pushFactory
+            .create(admin.getIdent(), testRepo, "original subject", "a", "a1")
+            .to("refs/for/master")
+            .getChange();
+    // Bootstrap: No NoteDb access as we expect it to use the index.
+    AcceptanceTestRequestScope.Context ctx = disableDb();
+    try {
+      changeRefCache.resetBootstrappedProjects();
+      assertUploadPackRefs(
+          "HEAD",
+          "refs/heads/master",
+          RefNames.changeMetaRef(change1.getId()),
+          change1.currentPatchSet().getId().toRefName());
+    } finally {
+      enableDb(ctx);
+    }
+
+    try (AutoCloseable ignored = disableChangeIndex()) {
+      // user can see public change
+      setApiUser(user);
+      assertUploadPackRefs(
+          "HEAD",
+          "refs/heads/master",
+          RefNames.changeMetaRef(change1.getId()),
+          change1.currentPatchSet().getId().toRefName());
+    }
+
+    // Delta reload: No index access as we expect it to use NoteDb.
+    setApiUser(admin);
+    gApi.changes().id(change1.getId().id).setPrivate(true);
+
+    try (AutoCloseable ignored = disableChangeIndex()) {
+      // user can't see private change from admin
+      setApiUser(user);
+      assertUploadPackRefs("HEAD", "refs/heads/master");
+    }
+
+    // admin adds the user as reviewer
+    setApiUser(admin);
+    gApi.changes().id(change1.getId().id).addReviewer(user.email);
+
+    try (AutoCloseable ignored = disableChangeIndex()) {
+      // Use can see private change
+      setApiUser(user);
+      assertUploadPackRefs(
+          "HEAD",
+          "refs/heads/master",
+          RefNames.changeMetaRef(change1.getId()),
+          change1.currentPatchSet().getId().toRefName());
+    }
+  }
+
+  private void assertUploadPackRefs(String... expectedRefs) throws Exception {
+    try (Repository repo = repoManager.openRepository(project)) {
+      assertRefs(repo, permissionBackend.user(user(user)).project(project), expectedRefs);
+    }
+  }
+
+  private void assertRefs(
+      Repository repo, PermissionBackend.ForProject forProject, String... expectedRefs)
+      throws Exception {
+    Map<String, Ref> all = getAllRefs(repo);
+    assertThat(forProject.filter(all, repo, RefFilterOptions.defaults()).keySet())
+        .containsExactlyElementsIn(expectedRefs);
+  }
+
+  private static Map<String, Ref> getAllRefs(Repository repo) throws IOException {
+    return repo.getRefDatabase()
+        .getRefs()
+        .stream()
+        .collect(toMap(Ref::getName, Function.identity()));
+  }
+}

javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java

@@ -25,7 +25,6 @@ import static java.util.stream.Collectors.toMap;
 import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableList;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
-import com.google.gerrit.acceptance.AcceptanceTestRequestScope;
 import com.google.gerrit.acceptance.GerritConfig;
 import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.acceptance.PushOneCommit;
@@ -357,7 +356,6 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
           repo,
           permissionBackend.user(user(user)).project(project),
           // Can't use stored values from the index so DB must be enabled.
-          false,
           "HEAD",
           psRef1,
           metaRef1,
@@ -377,10 +375,10 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
   @Test
   public void uploadPackSequencesWithAccessDatabase() throws Exception {
     try (Repository repo = repoManager.openRepository(allProjects)) {
-      assertRefs(repo, newFilter(allProjects, user), true);
+      assertRefs(repo, newFilter(allProjects, user));
 
       allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE);
-      assertRefs(repo, newFilter(allProjects, user), true, "refs/sequences/changes");
+      assertRefs(repo, newFilter(allProjects, user), "refs/sequences/changes");
     }
   }
 
@@ -728,29 +726,16 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
    */
   private void assertUploadPackRefs(String... expectedRefs) throws Exception {
     try (Repository repo = repoManager.openRepository(project)) {
-      assertRefs(repo, permissionBackend.user(user(user)).project(project), true, expectedRefs);
+      assertRefs(repo, permissionBackend.user(user(user)).project(project), expectedRefs);
     }
   }
 
   private void assertRefs(
-      Repository repo,
-      PermissionBackend.ForProject forProject,
-      boolean disableDb,
-      String... expectedRefs)
+      Repository repo, PermissionBackend.ForProject forProject, String... expectedRefs)
       throws Exception {
-    AcceptanceTestRequestScope.Context ctx = null;
-    if (disableDb) {
-      ctx = disableDb();
-    }
-    try {
     Map<String, Ref> all = getAllRefs(repo);
     assertThat(forProject.filter(all, repo, RefFilterOptions.defaults()).keySet())
         .containsExactlyElementsIn(expectedRefs);
-    } finally {
-      if (disableDb) {
-        enableDb(ctx);
-      }
-    }
   }
 
   private ReceiveCommitsAdvertiseRefsHook.Result getReceivePackRefs() throws Exception {