Revert "Rework SearchingChangeCacheImpl"
This reverts commit f5c89c1a6f01aee6295717fb785a4bfbacb8ce86.
Reason for revert:
Bootstrapping doesn't work because the index doesn't give us
all changes. In addition, we would have to filter way more
refs with this cache because the index-backed cache limitted
the ref filtering by recency (= you can't fetch a change from
say 2015) which is another performance bottleneck.
Change-Id: I447eaeb652e530ed560f31de15bb012912e14fff
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 01db7ce..e92b0bd 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -818,23 +818,18 @@
requires two HTTP requests, and this cache tries to carry state from
the first request into the second to ensure it can complete.
-cache `"change_refs"`::
-+
-Cache entries are used to compute change ref visibility efficiently. Entries
-contain the minimal required information for this task.
+cache `"changes"`::
+
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
-at maximum 1024 changes can be held in the cache.
+changes for up to 1024 projects can be held in the cache.
+
-Default value is 10.000.
+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.
+
-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.
+The cache should be flushed whenever the database changes table is modified
+outside of Gerrit.
cache `"diff"`::
+
diff --git a/java/com/google/gerrit/httpd/init/WebAppInitializer.java b/java/com/google/gerrit/httpd/init/WebAppInitializer.java
index a8552f4..aa5362b 100644
--- a/java/com/google/gerrit/httpd/init/WebAppInitializer.java
+++ b/java/com/google/gerrit/httpd/init/WebAppInitializer.java
@@ -66,9 +66,9 @@
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 @@
modules.add(cfgInjector.getInstance(GerritGlobalModule.class));
modules.add(new GerritApiModule());
modules.add(new PluginApiModule());
- modules.add(new ChangeRefCache.Module());
+ modules.add(new SearchingChangeCacheImpl.Module());
modules.add(new InternalAccountDirectory.Module());
modules.add(new DefaultPermissionBackendModule());
modules.add(new DefaultMemoryCacheModule());
diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java
index 3c4f89b..c280a2d 100644
--- a/java/com/google/gerrit/pgm/Daemon.java
+++ b/java/com/google/gerrit/pgm/Daemon.java
@@ -73,8 +73,8 @@
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 @@
modules.add(new GerritApiModule());
modules.add(new PluginApiModule());
- modules.add(new ChangeRefCache.Module());
+ modules.add(new SearchingChangeCacheImpl.Module(slave));
modules.add(new InternalAccountDirectory.Module());
modules.add(new DefaultPermissionBackendModule());
modules.add(new DefaultMemoryCacheModule());
diff --git a/java/com/google/gerrit/pgm/util/BatchProgramModule.java b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
index f263786..b0c1c25 100644
--- a/java/com/google/gerrit/pgm/util/BatchProgramModule.java
+++ b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
@@ -56,8 +56,8 @@
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 @@
// As Reindex is a batch program, don't assume the index is available for
// the change cache.
- bind(ChangeRefCache.class).toProvider(Providers.of(null));
+ bind(SearchingChangeCacheImpl.class).toProvider(Providers.of(null));
bind(new TypeLiteral<ImmutableSet<GroupReference>>() {})
.annotatedWith(AdministrateServerGroups.class)
diff --git a/java/com/google/gerrit/server/git/ChangeRefCache.java b/java/com/google/gerrit/server/git/ChangeRefCache.java
deleted file mode 100644
index dc1b946..0000000
--- a/java/com/google/gerrit/server/git/ChangeRefCache.java
+++ /dev/null
@@ -1,226 +0,0 @@
-// 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();
- }
-}
diff --git a/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java b/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java
new file mode 100644
index 0000000..0692ccf
--- /dev/null
+++ b/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java
@@ -0,0 +1,162 @@
+// 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);
+ }
+ }
+ }
+}
diff --git a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
index a2a0ef1..00df11a 100644
--- a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
+++ b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
@@ -14,6 +14,7 @@
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;
@@ -21,7 +22,9 @@
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;
@@ -38,11 +41,12 @@
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.ChangeRefCache;
+import com.google.gerrit.server.git.SearchingChangeCacheImpl;
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;
@@ -51,11 +55,10 @@
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;
@@ -71,7 +74,7 @@
private final TagCache tagCache;
private final ChangeNotes.Factory changeNotesFactory;
- @Nullable private final ChangeRefCache changeCache;
+ @Nullable private final SearchingChangeCacheImpl changeCache;
private final GroupCache groupCache;
private final PermissionBackend permissionBackend;
private final ProjectControl projectControl;
@@ -81,14 +84,14 @@
private final Counter0 fullFilterCount;
private final Counter0 skipFilterCount;
private final boolean skipFullRefEvaluationIfAllRefsAreVisible;
- private final Map<Change.Id, Branch.NameKey> visibleChanges;
- private final Set<Change.Id> inVisibleChanges;
+
+ private Map<Change.Id, Branch.NameKey> visibleChanges;
@Inject
DefaultRefFilter(
TagCache tagCache,
ChangeNotes.Factory changeNotesFactory,
- @Nullable ChangeRefCache changeCache,
+ @Nullable SearchingChangeCacheImpl changeCache,
GroupCache groupCache,
PermissionBackend permissionBackend,
@GerritServerConfig Config config,
@@ -118,9 +121,6 @@
"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,7 +161,6 @@
Map<String, Ref> result = new HashMap<>();
List<Ref> deferredTags = new ArrayList<>();
- changeCache.bootstrapIfNecessary(projectState.getNameKey());
for (Ref ref : refs.values()) {
String name = ref.getName();
@@ -300,70 +299,111 @@
}
private boolean visible(Repository repo, Change.Id changeId) throws PermissionBackendException {
- 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 {
- Ref metaRef = repo.exactRef(RefNames.changeMetaRef(changeId));
- if (metaRef == null) {
- logger.atWarning().log(
- "can't read change meta ref for change %s on project %s", project, changeId);
- inVisibleChanges.add(changeId);
- return false;
+ if (visibleChanges == null) {
+ if (changeCache == null) {
+ visibleChanges = visibleChangesByScan(repo);
+ } else {
+ visibleChanges = visibleChangesBySearch();
}
-
- ChangeData cd = changeCache.getChangeData(project, changeId, metaRef.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);
- if (id == null) {
- return false;
+ // Initialize if it wasn't yet
+ if (visibleChanges == null) {
+ visible(repo, id);
}
- if (!visible(repo, id)) {
- // Can't see the change, so can't see the edit.
+ if (id == null) {
return false;
}
if (user.isIdentifiedUser()
- && name.startsWith(RefNames.refsEditPrefix(user.asIdentifiedUser().getAccountId()))) {
- // Own edit
+ && name.startsWith(RefNames.refsEditPrefix(user.asIdentifiedUser().getAccountId()))
+ && visible(repo, id)) {
return true;
}
+ if (visibleChanges.containsKey(id)) {
+ try {
+ // Default to READ_PRIVATE_CHANGES as there is no special permission for reading edits.
+ permissionBackendForProject
+ .ref(visibleChanges.get(id).get())
+ .check(RefPermission.READ_PRIVATE_CHANGES);
+ return true;
+ } catch (AuthException e) {
+ 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 {
- // Default to READ_PRIVATE_CHANGES as there is no special permission for reading edits.
- permissionBackendForProject
- .ref(visibleChanges.get(id).get())
- .check(RefPermission.READ_PRIVATE_CHANGES);
- return true;
+ permissionBackendForProject.change(r.notes()).check(ChangePermission.READ);
+ return r.notes();
} catch (AuthException e) {
- return false;
+ // Skip.
}
+ return null;
}
private boolean isMetadata(String name) {
diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java
index 2a030ac..c8cea6f 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -58,10 +58,10 @@
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 @@
factory(PluginUser.Factory.class);
install(new PluginApiModule());
install(new DefaultPermissionBackendModule());
- install(new ChangeRefCache.Module());
+ install(new SearchingChangeCacheImpl.Module());
factory(GarbageCollection.Factory.class);
install(new AuditModule());
diff --git a/javatests/com/google/gerrit/acceptance/git/ChangeRefCacheIT.java b/javatests/com/google/gerrit/acceptance/git/ChangeRefCacheIT.java
deleted file mode 100644
index 10e701e..0000000
--- a/javatests/com/google/gerrit/acceptance/git/ChangeRefCacheIT.java
+++ /dev/null
@@ -1,251 +0,0 @@
-// 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.NoHttpd;
-import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
-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 ChangeRefCache changeRefCache;
- @Inject private PermissionBackend permissionBackend;
- @Inject private RequestScopeOperations requestScopeOperations;
-
- @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();
- try (AutoCloseable ignored = disableNoteDb()) {
- assertUploadPackRefs(
- "HEAD",
- "refs/heads/master",
- RefNames.changeMetaRef(change.getId()),
- change.currentPatchSet().getId().toRefName());
- }
- }
-
- /**
- * 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();
- try (AutoCloseable ignored = disableNoteDb()) {
- assertUploadPackRefs(
- "HEAD",
- "refs/heads/master",
- RefNames.changeMetaRef(change.getId()),
- change.currentPatchSet().getId().toRefName());
- }
-
- // No change since our first call, so this time we don't bootstrap or touch the NoteDb
- try (AutoCloseable ignored = disableChangeIndex();
- AutoCloseable ignored2 = disableNoteDb()) {
- assertUploadPackRefs(
- "HEAD",
- "refs/heads/master",
- RefNames.changeMetaRef(change.getId()),
- change.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 useIndexForBootstrappingAndDbForDeltaReload() throws Exception {
- ChangeData change1 = createChange().getChange();
- // Bootstrap: No NoteDb access as we expect it to use the index.
- changeRefCache.resetBootstrappedProjects();
- try (AutoCloseable ignored = disableNoteDb()) {
- 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.
- 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();
-
- // Bootstrap: No NoteDb access as we expect it to use the index.
- changeRefCache.resetBootstrappedProjects();
- try (AutoCloseable ignored = disableNoteDb()) {
- 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.
- 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.
- try (AutoCloseable ignored = disableNoteDb()) {
- changeRefCache.resetBootstrappedProjects();
- assertUploadPackRefs(
- "HEAD",
- "refs/heads/master",
- RefNames.changeMetaRef(change1.getId()),
- change1.currentPatchSet().getId().toRefName());
- }
-
- try (AutoCloseable ignored = disableChangeIndex()) {
- // user can see public change
- requestScopeOperations.setApiUser(user.getId());
- 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.
- requestScopeOperations.setApiUser(admin.getId());
- gApi.changes().id(change1.getId().id).setPrivate(true);
-
- try (AutoCloseable ignored = disableChangeIndex()) {
- // user can't see private change from admin
- requestScopeOperations.setApiUser(user.getId());
- assertUploadPackRefs("HEAD", "refs/heads/master");
- }
-
- // admin adds the user as reviewer
- requestScopeOperations.setApiUser(admin.getId());
- gApi.changes().id(change1.getId().id).addReviewer(user.email);
-
- try (AutoCloseable ignored = disableChangeIndex()) {
- // Use can see private change
- requestScopeOperations.setApiUser(user.getId());
- 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()));
- }
-}
diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
index 79a6957..91d5c32 100644
--- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
+++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
@@ -358,6 +358,7 @@
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 +378,10 @@
@Test
public void uploadPackSequencesWithAccessDatabase() throws Exception {
try (Repository repo = repoManager.openRepository(allProjects)) {
- assertRefs(repo, newFilter(allProjects, user));
+ assertRefs(repo, newFilter(allProjects, user), true);
allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE);
- assertRefs(repo, newFilter(allProjects, user), "refs/sequences/changes");
+ assertRefs(repo, newFilter(allProjects, user), true, "refs/sequences/changes");
}
}
@@ -728,16 +729,29 @@
*/
private void assertUploadPackRefs(String... expectedRefs) throws Exception {
try (Repository repo = repoManager.openRepository(project)) {
- assertRefs(repo, permissionBackend.user(user(user)).project(project), expectedRefs);
+ assertRefs(repo, permissionBackend.user(user(user)).project(project), true, expectedRefs);
}
}
private void assertRefs(
- Repository repo, PermissionBackend.ForProject forProject, String... expectedRefs)
+ Repository repo,
+ PermissionBackend.ForProject forProject,
+ boolean disableDb,
+ String... expectedRefs)
throws Exception {
- Map<String, Ref> all = getAllRefs(repo);
- assertThat(forProject.filter(all, repo, RefFilterOptions.defaults()).keySet())
- .containsExactlyElementsIn(expectedRefs);
+ AutoCloseable ctx = null;
+ if (disableDb) {
+ ctx = disableNoteDb();
+ }
+ try {
+ Map<String, Ref> all = getAllRefs(repo);
+ assertThat(forProject.filter(all, repo, RefFilterOptions.defaults()).keySet())
+ .containsExactlyElementsIn(expectedRefs);
+ } finally {
+ if (disableDb) {
+ ctx.close();
+ }
+ }
}
private ReceiveCommitsAdvertiseRefsHook.Result getReceivePackRefs() throws Exception {
