Introduce cache for open changes
Git-refs-filter plugin relies on change_notes when filtering for open
changes.
In order to speed up access to them, it is advised to enable caching for
change_notes, as documented in the README.md.
Whilst this improves the _accessing_ of the change note, it does nothing
to speed up the parsing of the change itself, in order to gather the
change status information, needed to assess whether the change is
actually open.
Introduce an additional in-memory cache to store previously computed
open/close change statuses to avoid processing them over and over
again.
Explicit cache invalidation is not necessary, since the change revision
is part of the cache key, so that previous entries automatically become
obsolete once a change status is updated.
Bug: Issue 15484
Change-Id: I36467dbd7c1ce646d97be410acbae564fa4a675f
diff --git a/README.md b/README.md
index c172399..5b6b123 100644
--- a/README.md
+++ b/README.md
@@ -27,6 +27,13 @@
git-refs-filter is slightly slower than using Git's hideRefs and it does require the configuration
of the change_notes cache in `gerrit.config` to avoid potentially high overhead.
+Additionally, this plugin uses an in-memory cache to store previously computed
+open/close change statuses to avoid processing them over and over again.
+
+Explicit invalidation of such cache is not necessary, since the change revision
+is part of the cache key, so that previous entries automatically become obsolete
+once a change status is updated.
+
### Gerrit ACLs
Use the Gerrit ACLs when you need to hide some of the refs on a per-project basis or when
diff --git a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ChangeOpenCache.java b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ChangeOpenCache.java
new file mode 100644
index 0000000..0587524
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ChangeOpenCache.java
@@ -0,0 +1,91 @@
+// Copyright (C) 2022 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.googlesource.gerrit.modules.gitrefsfilter;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.cache.CacheLoader;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.cache.CacheModule;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.inject.Inject;
+import com.google.inject.Module;
+import com.google.inject.Singleton;
+import com.google.inject.TypeLiteral;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
+
+public class ChangeOpenCache {
+ public static final String OPEN_CHANGES_CACHE = "open_changes";
+
+ public static Module module() {
+ return new CacheModule() {
+ @Override
+ protected void configure() {
+ cache(OPEN_CHANGES_CACHE, Key.class, new TypeLiteral<Boolean>() {}).loader(Loader.class);
+ }
+ };
+ }
+
+ @AutoValue
+ public abstract static class Key {
+ /* Note: `repo` and `changeId` need to be part of the cache key because the
+ Loader requires them in order to compute the relevant changeNote to extract
+ the change openness status from. */
+ public abstract Repository repo();
+
+ public abstract Change.Id changeId();
+
+ @Nullable
+ public abstract ObjectId changeRevision();
+
+ public abstract Project.NameKey project();
+
+ static Key create(
+ Repository repo,
+ Change.Id changeId,
+ @Nullable ObjectId changeRevision,
+ Project.NameKey project) {
+ return new AutoValue_ChangeOpenCache_Key(repo, changeId, changeRevision, project);
+ }
+ }
+
+ @Singleton
+ static class Loader extends CacheLoader<Key, Boolean> {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private final ChangeNotes.Factory changeNotesFactory;
+
+ @Inject
+ Loader(ChangeNotes.Factory changeNotesFactory) {
+ this.changeNotesFactory = changeNotesFactory;
+ }
+
+ @Override
+ public Boolean load(Key key) throws Exception {
+ try {
+ ChangeNotes changeNotes =
+ changeNotesFactory.createChecked(
+ key.repo(), key.project(), key.changeId(), key.changeRevision());
+ return changeNotes.getChange().getStatus().isOpen();
+ } catch (NoSuchChangeException e) {
+ logger.atFine().withCause(e).log(
+ "Change %d does not exist: hiding from the advertised refs", key.changeId());
+ return false;
+ }
+ }
+ }
+}
diff --git a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ForProjectWrapper.java b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ForProjectWrapper.java
index 23c26ac..1c59ab7 100644
--- a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ForProjectWrapper.java
+++ b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ForProjectWrapper.java
@@ -14,24 +14,28 @@
package com.googlesource.gerrit.modules.gitrefsfilter;
+import static com.googlesource.gerrit.modules.gitrefsfilter.ChangeOpenCache.OPEN_CHANGES_CACHE;
+
+import com.google.common.cache.LoadingCache;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.access.CoreOrPluginProjectPermission;
import com.google.gerrit.extensions.conditions.BooleanCondition;
import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackend.ForProject;
import com.google.gerrit.server.permissions.PermissionBackend.ForRef;
import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
+import com.google.inject.name.Named;
import java.util.Collection;
import java.util.Map;
import java.util.Set;
+import java.util.concurrent.ExecutionException;
import java.util.stream.Collectors;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
@@ -40,9 +44,9 @@
public class ForProjectWrapper extends ForProject {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private final LoadingCache<ChangeOpenCache.Key, Boolean> openChangesCache;
private final ForProject defaultForProject;
private final Project.NameKey project;
- private final ChangeNotes.Factory changeNotesFactory;
private final FilterRefsConfig config;
public interface Factory {
@@ -51,13 +55,13 @@
@Inject
public ForProjectWrapper(
- ChangeNotes.Factory changeNotesFactory,
FilterRefsConfig config,
+ @Named(OPEN_CHANGES_CACHE) LoadingCache<ChangeOpenCache.Key, Boolean> openChangesCache,
@Assisted ForProject defaultForProject,
@Assisted Project.NameKey project) {
+ this.openChangesCache = openChangesCache;
this.defaultForProject = defaultForProject;
this.project = project;
- this.changeNotesFactory = changeNotesFactory;
this.config = config;
}
@@ -115,15 +119,15 @@
return isChangeRef(changeKey) && changeKey.endsWith("/meta");
}
- private boolean isOpen(Repository repo, Change.Id changeId, ObjectId changeRevision) {
+ private boolean isOpen(Repository repo, Change.Id changeId, @Nullable ObjectId changeRevision) {
try {
- ChangeNotes changeNotes =
- changeNotesFactory.createChecked(repo, project, changeId, changeRevision);
- return changeNotes.getChange().getStatus().isOpen();
- } catch (NoSuchChangeException e) {
- logger.atFine().withCause(e).log(
- "Change %d does not exist: hiding from the advertised refs", changeId);
- return false;
+ return openChangesCache.get(
+ ChangeOpenCache.Key.create(repo, changeId, changeRevision, project));
+ } catch (ExecutionException e) {
+ logger.atWarning().withCause(e).log(
+ "Error getting change '%d' from the cache. Do not hide from the advertised refs",
+ changeId);
+ return true;
}
}
diff --git a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/RefsFilterModule.java b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/RefsFilterModule.java
index 98ce862..9a3f65f 100644
--- a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/RefsFilterModule.java
+++ b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/RefsFilterModule.java
@@ -41,5 +41,7 @@
.annotatedWith(Exports.named(FilterRefsCapability.HIDE_CLOSED_CHANGES_REFS))
.to(FilterRefsCapability.class)
.in(Scopes.SINGLETON);
+
+ install(ChangeOpenCache.module());
}
}
diff --git a/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/AbstractGitDaemonTest.java b/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/AbstractGitDaemonTest.java
index ea1d61e..c58f0b8 100644
--- a/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/AbstractGitDaemonTest.java
+++ b/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/AbstractGitDaemonTest.java
@@ -43,11 +43,12 @@
@Inject private RequestScopeOperations requestScopeOperations;
@Inject private ProjectOperations projectOperations;
- protected void createChangeAndAbandon() throws Exception, RestApiException {
+ protected int createChangeAndAbandon() throws Exception, RestApiException {
requestScopeOperations.setApiUser(admin.id());
createChange();
int changeNum = changeNumOfRef(getChangesRefsAs(admin).get(0));
gApi.changes().id(changeNum).abandon();
+ return changeNum;
}
protected void createFilteredRefsGroup() throws Exception {
diff --git a/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/GitRefsFilterTest.java b/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/GitRefsFilterTest.java
index 957d4f7..864469a 100644
--- a/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/GitRefsFilterTest.java
+++ b/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/GitRefsFilterTest.java
@@ -15,7 +15,9 @@
package com.googlesource.gerrit.libmodule.plugins.test;
import static com.google.common.truth.Truth.assertThat;
+import static com.googlesource.gerrit.modules.gitrefsfilter.ChangeOpenCache.OPEN_CHANGES_CACHE;
+import com.google.common.cache.LoadingCache;
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.Sandboxed;
@@ -23,12 +25,17 @@
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.RefNames;
import com.google.inject.Inject;
import com.google.inject.Module;
+import com.google.inject.name.Named;
+import com.googlesource.gerrit.modules.gitrefsfilter.ChangeOpenCache;
import com.googlesource.gerrit.modules.gitrefsfilter.RefsFilterModule;
import java.io.IOException;
+import java.util.ArrayList;
import java.util.List;
+import java.util.Map;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
@@ -36,6 +43,7 @@
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.transport.FetchResult;
import org.eclipse.jgit.util.FS;
import org.junit.Before;
@@ -46,6 +54,9 @@
public class GitRefsFilterTest extends AbstractGitDaemonTest {
@Inject private RequestScopeOperations requestScopeOperations;
+ @Inject
+ private @Named(OPEN_CHANGES_CACHE) LoadingCache<ChangeOpenCache.Key, Boolean> changeOpenCache;
+
@Override
public Module createModule() {
return new RefsFilterModule();
@@ -116,6 +127,44 @@
.isNotEmpty();
}
+ @Test
+ public void testShouldCacheChangeIsClosedWhenAbandoned() throws Exception {
+ Change.Id changeId = Change.id(createChangeAndAbandon());
+ Ref metaRef = getMetaId(changeId);
+
+ assertThat(getRefs(cloneProjectChangesRefs(user))).isEmpty();
+
+ assertThat(changeOpenCache.asMap().size()).isEqualTo(1);
+
+ Map.Entry<ChangeOpenCache.Key, Boolean> cacheEntry =
+ new ArrayList<>(changeOpenCache.asMap().entrySet()).get(0);
+
+ assertThat(cacheEntry.getKey().project()).isEqualTo(project);
+ assertThat(cacheEntry.getKey().changeId()).isEqualTo(changeId);
+ assertThat(cacheEntry.getKey().changeRevision()).isEqualTo(metaRef.getObjectId());
+ assertThat(cacheEntry.getValue()).isFalse();
+ }
+
+ @Test
+ public void testShouldCacheWhenChangeIsOpen() throws Exception {
+ createChange();
+ List<Ref> refs = getRefs(cloneProjectChangesRefs(user));
+
+ assertThat(refs).isNotEmpty();
+
+ Change.Id changeId = Change.id(changeNumOfRef(refs.get(0)));
+
+ assertThat(changeOpenCache.asMap().size()).isEqualTo(1);
+
+ Map.Entry<ChangeOpenCache.Key, Boolean> cacheEntry =
+ new ArrayList<>(changeOpenCache.asMap().entrySet()).get(0);
+
+ assertThat(cacheEntry.getKey().project()).isEqualTo(project);
+ assertThat(cacheEntry.getKey().changeId()).isEqualTo(changeId);
+ assertThat(cacheEntry.getKey().changeRevision()).isEqualTo(getMetaId(changeId).getObjectId());
+ assertThat(cacheEntry.getValue()).isTrue();
+ }
+
protected Stream<String> fetchAllRefs(TestAccount testAccount) throws Exception {
DfsRepositoryDescription desc = new DfsRepositoryDescription("clone of " + project.get());
@@ -137,4 +186,10 @@
throws IOException {
return repo.getRepository().getRefDatabase().getRefsByPrefix(prefix);
}
+
+ private Ref getMetaId(Change.Id changeId) throws Exception {
+ try (Repository r = repoManager.openRepository(project)) {
+ return r.exactRef(RefNames.changeMetaRef(changeId));
+ }
+ }
}
