summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorMartin Fick <martin.fick@linaro.org>2023-08-09 10:04:58 -0600
committerMartin Fick <martin.fick@linaro.org>2023-09-28 17:03:23 -0600
commit9bfeb0ea79e874bbadeb583492c0e5e91601f463 (patch)
tree7aeb70ce2056a0bd62d2d7fea8bb80e3a57672fb
parenta7401b0131a8899295f91ad96b3de47e12cb4ad6 (diff)
Add in memory change data cache by project
This cache only stores the data needed to speedup ref advertisements. The index is used to populate this cache when it is available otherwise it falls back to scanning noteDb (generally on replicas). This cache is most useful when using 'git upload-pack' with projects with large change counts on replicas. The following times were seen when running 'git ls-remote' on a project with around 380K changes stored on NFS: With ES index (primary): Before this change: (cold 1m3s) 12-17s (with large enough "changes" cache) After this change: (cold 1m3s) 12-17s (with large enough "changes" cache) No Index (replica): Before this change: (cold ~4m) ~4m After this change: (cold 2m25s) 12-17s Although the numbers above do not show an improvement on "best" times for primaries using ES, that's because those numbers represent the times achieved with the existing "changes" cache enabled. However, since the existing "changes" cache is disabled by default, most sites will see an improvement after this change with 'git upload-pack' on primaries also, and additionally with 'git receive-pack'. Thus the warm times after this change are more achievable and more likely to stay fast, even when other projects are accessed in between. Before this change it is harder to achieve and maintain the warm times. The existing "changes" cache can also is used to help cache changes by project, however, it is documented as not being appropriate for multi-primary setups since it will be out of date if alterations are made to changes outside of the current server. The existing cache is also not useable on replicas where there is no index. It is noteworthy that the existing cache provide value beyond just its ability to cache results since loading change data from the index (even with ES), is faster than loading change data via a noteDb scan. Similar to the existing cache, the new cache is able to take advantage of the index when it is available on primaries, however it is actually multi-primary friendly because it stores the meta-ref id for each change and out-of-date changes are detected on each cache fetch resulting in the cache being incrementally kept up-to-date. The new cache is also useable on replicas by also falling back to a noteDb scan when the index is not available, however here too it is able to incrementally stay up to date after the first scan. The new cache is much more space efficient than the existing "changes" cache as it takes advantage of the fact that determining visibility for public changes requires access only to a change's destination branch and that many changes are generally destined for each branch. So although the existing cache is fairly compact, this new cache stores even fewer change data fields. This makes it likely possible to keep all the projects on a server cached in memory, even on servers with large projects and change counts. It may make sense to eventually remove the existing changes cache. Release-Notes: Add cache to speedup ref advertisements (particularly on replicas) and receive-pack Forward-Compatible: checked Change-Id: Ib9c77ee03d9012cdeb6ad05eed3492c3009e4334
-rw-r--r--Documentation/config-gerrit.txt7
-rw-r--r--java/com/google/gerrit/httpd/init/WebAppInitializer.java4
-rw-r--r--java/com/google/gerrit/pgm/Daemon.java7
-rw-r--r--java/com/google/gerrit/pgm/util/BatchProgramModule.java8
-rw-r--r--java/com/google/gerrit/server/git/ChangesByProjectCache.java63
-rw-r--r--java/com/google/gerrit/server/git/ChangesByProjectCacheImpl.java356
-rw-r--r--java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java38
-rw-r--r--java/com/google/gerrit/server/notedb/ChangeNotes.java6
-rw-r--r--java/com/google/gerrit/server/permissions/ChangeControl.java2
-rw-r--r--java/com/google/gerrit/server/permissions/PermissionBackend.java2
-rw-r--r--java/com/google/gerrit/server/permissions/ProjectControl.java16
-rw-r--r--java/com/google/gerrit/server/permissions/VisibleChangesCache.java87
-rw-r--r--java/com/google/gerrit/server/query/change/ChangeData.java93
-rw-r--r--java/com/google/gerrit/testing/InMemoryModule.java4
14 files changed, 570 insertions, 123 deletions
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 5d30fd8581..9e36313d97 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -994,6 +994,13 @@ or multiple replica nodes.
The cache should be flushed whenever NoteDb change metadata in a repository is
modified outside of Gerrit.
+cache `"changes_by_project"`::
++
+Ideally, the memorylimit of this cache is large enough to cover all projects.
+This should significantly speed up change ref advertisements and git pushes,
+especially for projects with lots of changes, and particularly on replicas
+where there is no index.
+
cache `"git_modified_files"`::
+
Each item caches the list of git modified files between two git trees
diff --git a/java/com/google/gerrit/httpd/init/WebAppInitializer.java b/java/com/google/gerrit/httpd/init/WebAppInitializer.java
index dbda12ae15..6488f2e934 100644
--- a/java/com/google/gerrit/httpd/init/WebAppInitializer.java
+++ b/java/com/google/gerrit/httpd/init/WebAppInitializer.java
@@ -74,9 +74,9 @@ import com.google.gerrit.server.config.SitePath;
import com.google.gerrit.server.config.SysExecutorModule;
import com.google.gerrit.server.events.EventBroker.EventBrokerModule;
import com.google.gerrit.server.events.StreamEventsApiListener.StreamEventsApiListenerModule;
+import com.google.gerrit.server.git.ChangesByProjectCache;
import com.google.gerrit.server.git.GarbageCollectionModule;
import com.google.gerrit.server.git.GitRepositoryManagerModule;
-import com.google.gerrit.server.git.SearchingChangeCacheImpl.SearchingChangeCacheImplModule;
import com.google.gerrit.server.git.SystemReaderInstaller;
import com.google.gerrit.server.git.WorkQueue.WorkQueueModule;
import com.google.gerrit.server.index.IndexModule;
@@ -309,7 +309,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 SearchingChangeCacheImplModule());
+ modules.add(new ChangesByProjectCache.Module(ChangesByProjectCache.UseIndex.TRUE, config));
modules.add(new InternalAccountDirectoryModule());
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 1f19712b46..3b31f4b508 100644
--- a/java/com/google/gerrit/pgm/Daemon.java
+++ b/java/com/google/gerrit/pgm/Daemon.java
@@ -83,8 +83,8 @@ import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SysExecutorModule;
import com.google.gerrit.server.events.EventBroker.EventBrokerModule;
import com.google.gerrit.server.events.StreamEventsApiListener.StreamEventsApiListenerModule;
+import com.google.gerrit.server.git.ChangesByProjectCache;
import com.google.gerrit.server.git.GarbageCollectionModule;
-import com.google.gerrit.server.git.SearchingChangeCacheImpl.SearchingChangeCacheImplModule;
import com.google.gerrit.server.git.WorkQueue.WorkQueueModule;
import com.google.gerrit.server.group.PeriodicGroupIndexer.PeriodicGroupIndexerModule;
import com.google.gerrit.server.index.AbstractIndexModule;
@@ -453,7 +453,10 @@ public class Daemon extends SiteProgram {
modules.add(new GerritApiModule());
modules.add(new PluginApiModule());
- modules.add(new SearchingChangeCacheImplModule(replica));
+ modules.add(
+ new ChangesByProjectCache.Module(
+ replica ? ChangesByProjectCache.UseIndex.FALSE : ChangesByProjectCache.UseIndex.TRUE,
+ config));
modules.add(new InternalAccountDirectoryModule());
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 c397539b06..48392b59a1 100644
--- a/java/com/google/gerrit/pgm/util/BatchProgramModule.java
+++ b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
@@ -65,9 +65,9 @@ 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.extensions.events.WorkInProgressStateChanged;
+import com.google.gerrit.server.git.ChangesByProjectCache;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.git.PureRevertCache;
-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;
@@ -158,10 +158,6 @@ public class BatchProgramModule extends FactoryModule {
factory(PatchSetInserter.Factory.class);
factory(RebaseChangeOp.Factory.class);
- // 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(new TypeLiteral<ImmutableSet<GroupReference>>() {})
.annotatedWith(AdministrateServerGroups.class)
.toInstance(ImmutableSet.of());
@@ -173,6 +169,8 @@ public class BatchProgramModule extends FactoryModule {
.toInstance(Collections.emptySet());
modules.add(new BatchGitModule());
+ modules.add(
+ new ChangesByProjectCache.Module(ChangesByProjectCache.UseIndex.FALSE, getConfig()));
modules.add(new DefaultPermissionBackendModule());
modules.add(new DefaultMemoryCacheModule());
modules.add(new H2CacheModule());
diff --git a/java/com/google/gerrit/server/git/ChangesByProjectCache.java b/java/com/google/gerrit/server/git/ChangesByProjectCache.java
new file mode 100644
index 0000000000..e4762087be
--- /dev/null
+++ b/java/com/google/gerrit/server/git/ChangesByProjectCache.java
@@ -0,0 +1,63 @@
+// Copyright (C) 2023 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.gerrit.entities.Project;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.inject.AbstractModule;
+import java.io.IOException;
+import java.util.Collection;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.Repository;
+
+public interface ChangesByProjectCache {
+ public enum UseIndex {
+ TRUE,
+ FALSE;
+ }
+
+ public static class Module extends AbstractModule {
+ private UseIndex useIndex;
+ private @GerritServerConfig Config config;
+
+ public Module(UseIndex useIndex, @GerritServerConfig Config config) {
+ this.useIndex = useIndex;
+ this.config = config;
+ }
+
+ @Override
+ protected void configure() {
+ boolean searchingCacheEnabled =
+ config.getLong("cache", SearchingChangeCacheImpl.ID_CACHE, "memoryLimit", 0) > 0;
+ if (searchingCacheEnabled && UseIndex.TRUE.equals(useIndex)) {
+ install(new SearchingChangeCacheImpl.SearchingChangeCacheImplModule());
+ } else {
+ bind(UseIndex.class).toInstance(useIndex);
+ install(new ChangesByProjectCacheImpl.Module());
+ }
+ }
+ }
+
+ /**
+ * get changeDatas for the project
+ *
+ * @param project project to read.
+ * @param repository repository for the project to read.
+ * @return Collection of known changes; empty if no changes.
+ */
+ Collection<ChangeData> getChangeDatas(Project.NameKey project, Repository repo)
+ throws IOException;
+}
diff --git a/java/com/google/gerrit/server/git/ChangesByProjectCacheImpl.java b/java/com/google/gerrit/server/git/ChangesByProjectCacheImpl.java
new file mode 100644
index 0000000000..2d15a500d2
--- /dev/null
+++ b/java/com/google/gerrit/server/git/ChangesByProjectCacheImpl.java
@@ -0,0 +1,356 @@
+// Copyright (C) 2023 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.Cache;
+import com.google.common.cache.Weigher;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.ReviewerSet;
+import com.google.gerrit.server.cache.CacheModule;
+import com.google.gerrit.server.git.ChangesByProjectCache.UseIndex;
+import com.google.gerrit.server.index.change.ChangeField;
+import com.google.gerrit.server.logging.Metadata;
+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.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+import com.google.inject.name.Named;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ExecutionException;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
+
+/**
+ * Lightweight cache of changes in each project.
+ *
+ * <p>This cache is intended to be used when filtering references and stores only the minimal fields
+ * required for a read permission check.
+ */
+@Singleton
+public class ChangesByProjectCacheImpl implements ChangesByProjectCache {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private static final String CACHE_NAME = "changes_by_project";
+
+ public static class Module extends CacheModule {
+ @Override
+ protected void configure() {
+ cache(CACHE_NAME, Project.NameKey.class, CachedProjectChanges.class)
+ .weigher(ChangesByProjetCacheWeigher.class);
+ bind(ChangesByProjectCache.class).to(ChangesByProjectCacheImpl.class);
+ }
+ }
+
+ private final Cache<Project.NameKey, CachedProjectChanges> cache;
+ private final ChangeData.Factory cdFactory;
+ private final UseIndex useIndex;
+ private final Provider<InternalChangeQuery> queryProvider;
+
+ @Inject
+ ChangesByProjectCacheImpl(
+ @Named(CACHE_NAME) Cache<Project.NameKey, CachedProjectChanges> cache,
+ ChangeData.Factory cdFactory,
+ UseIndex useIndex,
+ Provider<InternalChangeQuery> queryProvider) {
+ this.cache = cache;
+ this.cdFactory = cdFactory;
+ this.useIndex = useIndex;
+ this.queryProvider = queryProvider;
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public Collection<ChangeData> getChangeDatas(Project.NameKey project, Repository repo)
+ throws IOException {
+ CachedProjectChanges projectChanges = cache.getIfPresent(project);
+ if (projectChanges != null) {
+ return projectChanges.getUpdatedChangeDatas(
+ project, repo, cdFactory, ChangeNotes.Factory.scanChangeIds(repo), "Updating");
+ }
+ if (UseIndex.TRUE.equals(useIndex)) {
+ return queryChangeDatasAndLoad(project);
+ }
+ return scanChangeDatasAndLoad(project, repo);
+ }
+
+ private Collection<ChangeData> scanChangeDatasAndLoad(Project.NameKey project, Repository repo)
+ throws IOException {
+ CachedProjectChanges ours = new CachedProjectChanges();
+ CachedProjectChanges projectChanges = ours;
+ try {
+ projectChanges = cache.get(project, () -> ours);
+ } catch (ExecutionException e) {
+ logger.atWarning().withCause(e).log("Cannot load %s for %s", CACHE_NAME, project.get());
+ }
+ return projectChanges.getUpdatedChangeDatas(
+ project,
+ repo,
+ cdFactory,
+ ChangeNotes.Factory.scanChangeIds(repo),
+ ours == projectChanges ? "Scanning" : "Updating");
+ }
+
+ private Collection<ChangeData> queryChangeDatasAndLoad(Project.NameKey project) {
+ Collection<ChangeData> cds = queryChangeDatas(project);
+ cache.put(project, new CachedProjectChanges(cds));
+ return cds;
+ }
+
+ private Collection<ChangeData> queryChangeDatas(Project.NameKey project) {
+ try (TraceTimer timer =
+ TraceContext.newTimer(
+ "Querying changes of project", Metadata.builder().projectName(project.get()).build())) {
+ return queryProvider
+ .get()
+ .setRequestedFields(ChangeField.CHANGE, ChangeField.REVIEWER, ChangeField.REF_STATE)
+ .byProject(project);
+ }
+ }
+
+ private static class CachedProjectChanges {
+ Map<String, Map<Change.Id, ObjectId>> metaObjectIdByNonPrivateChangeByBranch =
+ new ConcurrentHashMap<>(); // BranchNameKey "normalized" to a String to dedup project
+ Map<Change.Id, PrivateChange> privateChangeById = new ConcurrentHashMap<>();
+
+ public CachedProjectChanges() {}
+
+ public CachedProjectChanges(Collection<ChangeData> cds) {
+ cds.stream().forEach(cd -> insert(cd));
+ }
+
+ public Collection<ChangeData> getUpdatedChangeDatas(
+ Project.NameKey project,
+ Repository repo,
+ ChangeData.Factory cdFactory,
+ Map<Change.Id, ObjectId> metaObjectIdByChange,
+ String operation) {
+ try (TraceTimer timer =
+ TraceContext.newTimer(
+ operation + " changes of project",
+ Metadata.builder().projectName(project.get()).build())) {
+ Map<Change.Id, ChangeData> cachedCdByChange = getChangeDataByChange(project, cdFactory);
+ List<ChangeData> cds = new ArrayList<>();
+ for (Map.Entry<Change.Id, ObjectId> e : metaObjectIdByChange.entrySet()) {
+ Change.Id id = e.getKey();
+ ChangeData cached = cachedCdByChange.get(id);
+ ChangeData cd = cached;
+ try {
+ if (cd == null || !cached.metaRevisionOrThrow().equals(e.getValue())) {
+ cd = cdFactory.create(project, id);
+ update(cached, cd);
+ }
+ } catch (Exception ex) {
+ // Do not let a bad change prevent other changes from being available.
+ logger.atFinest().withCause(ex).log("Can't load changeData for %s", id);
+ }
+ cds.add(cd);
+ }
+ return cds;
+ }
+ }
+
+ public CachedProjectChanges update(ChangeData old, ChangeData updated) {
+ if (old != null) {
+ if (old.isPrivateOrThrow()) {
+ privateChangeById.remove(old.getId());
+ } else {
+ Map<Change.Id, ObjectId> metaObjectIdByNonPrivateChange =
+ metaObjectIdByNonPrivateChangeByBranch.get(old.branchOrThrow().branch());
+ if (metaObjectIdByNonPrivateChange != null) {
+ metaObjectIdByNonPrivateChange.remove(old.getId());
+ }
+ }
+ }
+ return insert(updated);
+ }
+
+ public CachedProjectChanges insert(ChangeData cd) {
+ if (cd.isPrivateOrThrow()) {
+ privateChangeById.put(
+ cd.getId(),
+ new AutoValue_ChangesByProjectCacheImpl_PrivateChange(
+ cd.change(), cd.reviewers(), cd.metaRevisionOrThrow()));
+ } else {
+ metaObjectIdByNonPrivateChangeByBranch
+ .computeIfAbsent(cd.branchOrThrow().branch(), b -> new ConcurrentHashMap<>())
+ .put(cd.getId(), cd.metaRevisionOrThrow());
+ }
+ return this;
+ }
+
+ public Map<Change.Id, ChangeData> getChangeDataByChange(
+ Project.NameKey project, ChangeData.Factory cdFactory) {
+ Map<Change.Id, ChangeData> cdByChange = new HashMap<>(privateChangeById.size());
+ for (PrivateChange pc : privateChangeById.values()) {
+ try {
+ ChangeData cd = cdFactory.create(pc.change());
+ cd.setReviewers(pc.reviewers());
+ cd.setMetaRevision(pc.metaRevision());
+ cdByChange.put(cd.getId(), cd);
+ } catch (Exception ex) {
+ // Do not let a bad change prevent other changes from being available.
+ logger.atFinest().withCause(ex).log("Can't load changeData for %s", pc.change().getId());
+ }
+ }
+
+ for (Map.Entry<String, Map<Change.Id, ObjectId>> e :
+ metaObjectIdByNonPrivateChangeByBranch.entrySet()) {
+ BranchNameKey branch = BranchNameKey.create(project, e.getKey());
+ for (Map.Entry<Change.Id, ObjectId> e2 : e.getValue().entrySet()) {
+ Change.Id id = e2.getKey();
+ try {
+ cdByChange.put(id, cdFactory.createNonPrivate(branch, id, e2.getValue()));
+ } catch (Exception ex) {
+ // Do not let a bad change prevent other changes from being available.
+ logger.atFinest().withCause(ex).log("Can't load changeData for %s", id);
+ }
+ }
+ }
+ return cdByChange;
+ }
+
+ public int weigh() {
+ int size = 0;
+ size += 24 * 2; // guess at basic ConcurrentHashMap overhead * 2
+ for (Map.Entry<String, Map<Change.Id, ObjectId>> e :
+ metaObjectIdByNonPrivateChangeByBranch.entrySet()) {
+ size += JavaWeights.REFERENCE + e.getKey().length();
+ size +=
+ e.getValue().size()
+ * (JavaWeights.REFERENCE
+ + JavaWeights.OBJECT // Map.Entry
+ + JavaWeights.REFERENCE
+ + GerritWeights.CHANGE_NUM
+ + JavaWeights.REFERENCE
+ + GerritWeights.OBJECTID);
+ }
+ for (Map.Entry<Change.Id, PrivateChange> e : privateChangeById.entrySet()) {
+ size += JavaWeights.REFERENCE + GerritWeights.CHANGE_NUM;
+ size += JavaWeights.REFERENCE + e.getValue().weigh();
+ }
+ return size;
+ }
+ }
+
+ @AutoValue
+ abstract static class PrivateChange {
+ // Fields needed to serve permission checks on private Changes
+ abstract Change change();
+
+ @Nullable
+ abstract ReviewerSet reviewers();
+
+ abstract ObjectId metaRevision(); // Needed to confirm whether up-to-date
+
+ public int weigh() {
+ int size = 0;
+ size += JavaWeights.OBJECT; // this
+ size += JavaWeights.REFERENCE + weigh(change());
+ size += JavaWeights.REFERENCE + weigh(reviewers());
+ size += JavaWeights.REFERENCE + GerritWeights.OBJECTID; // metaRevision
+ return size;
+ }
+
+ private static int weigh(Change c) {
+ int size = 0;
+ size += JavaWeights.OBJECT; // change
+ size += JavaWeights.REFERENCE + GerritWeights.KEY_INT; // changeId
+ size += JavaWeights.REFERENCE + JavaWeights.OBJECT + 40; // changeKey;
+ size += JavaWeights.REFERENCE + GerritWeights.TIMESTAMP; // createdOn;
+ size += JavaWeights.REFERENCE + GerritWeights.TIMESTAMP; // lastUpdatedOn;
+ size += JavaWeights.REFERENCE + GerritWeights.ACCOUNT_ID; // owner;
+ size +=
+ JavaWeights.REFERENCE
+ + c.getDest().project().get().length()
+ + c.getDest().branch().length();
+ size += JavaWeights.CHAR; // status;
+ size += JavaWeights.INT; // currentPatchSetId;
+ size += JavaWeights.REFERENCE + c.getSubject().length();
+ size += JavaWeights.REFERENCE + (c.getTopic() == null ? 0 : c.getTopic().length());
+ size +=
+ JavaWeights.REFERENCE
+ + (c.getOriginalSubject().equals(c.getSubject()) ? 0 : c.getSubject().length());
+ size +=
+ JavaWeights.REFERENCE + (c.getSubmissionId() == null ? 0 : c.getSubmissionId().length());
+ size += JavaWeights.REFERENCE + GerritWeights.ACCOUNT_ID; // assignee;
+ size += JavaWeights.REFERENCE + JavaWeights.BOOLEAN; // isPrivate;
+ size += JavaWeights.REFERENCE + JavaWeights.BOOLEAN; // workInProgress;
+ size += JavaWeights.REFERENCE + JavaWeights.BOOLEAN; // reviewStarted;
+ size += JavaWeights.REFERENCE + GerritWeights.CHANGE_NUM; // revertOf;
+ size += JavaWeights.REFERENCE + GerritWeights.PACTCH_SET_ID; // cherryPickOf;
+ return size;
+ }
+
+ private static int weigh(ReviewerSet rs) {
+ int size = 0;
+ size += JavaWeights.OBJECT; // ReviewerSet
+ size += JavaWeights.REFERENCE; // table
+ size +=
+ rs.asTable().cellSet().size()
+ * (JavaWeights.OBJECT // cell (at least one object)
+ + JavaWeights.REFERENCE // ReviewerStateInternal
+ + (JavaWeights.REFERENCE + GerritWeights.ACCOUNT_ID)
+ + (JavaWeights.REFERENCE + GerritWeights.TIMESTAMP));
+ size += JavaWeights.REFERENCE; // accounts
+ return size;
+ }
+ }
+
+ private static class ChangesByProjetCacheWeigher
+ implements Weigher<Project.NameKey, CachedProjectChanges> {
+ @Override
+ public int weigh(Project.NameKey project, CachedProjectChanges changes) {
+ int size = 0;
+ size += project.get().length();
+ size += changes.weigh();
+ return size;
+ }
+ }
+
+ private static class GerritWeights {
+ public static final int KEY_INT = JavaWeights.OBJECT + JavaWeights.INT; // IntKey
+ public static final int CHANGE_NUM = KEY_INT;
+ public static final int ACCOUNT_ID = KEY_INT;
+ public static final int PACTCH_SET_ID =
+ JavaWeights.OBJECT
+ + (JavaWeights.REFERENCE + GerritWeights.CHANGE_NUM) // PatchSet.Id.changeId
+ + JavaWeights.INT; // PatchSet.Id patch_num;
+ public static final int TIMESTAMP = JavaWeights.OBJECT + 8; // Timestamp
+ public static final int OBJECTID = JavaWeights.OBJECT + (5 * JavaWeights.INT); // (w1-w5)
+ }
+
+ private static class JavaWeights {
+ public static final int BOOLEAN = 1;
+ public static final int CHAR = 1;
+ public static final int INT = 4;
+ public static final int OBJECT = 16;
+ public static final int REFERENCE = 8;
+ }
+}
diff --git a/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java b/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java
index ff5bcc2f8f..b7731bf93b 100644
--- a/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java
+++ b/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java
@@ -40,11 +40,12 @@ 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.Collection;
import java.util.Collections;
import java.util.List;
import java.util.concurrent.ExecutionException;
+import org.eclipse.jgit.lib.Repository;
/**
* Cache based on an index query of the most recent changes. The number of cached items depends on
@@ -54,35 +55,22 @@ import java.util.concurrent.ExecutionException;
* fraction of all changes. These are the changes that were modified last.
*/
@Singleton
-public class SearchingChangeCacheImpl implements GitReferenceUpdatedListener {
+public class SearchingChangeCacheImpl
+ implements ChangesByProjectCache, GitReferenceUpdatedListener {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
static final String ID_CACHE = "changes";
public static class SearchingChangeCacheImplModule extends CacheModule {
- private final boolean slave;
-
- public SearchingChangeCacheImplModule() {
- this(false);
- }
-
- public SearchingChangeCacheImplModule(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);
+ 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);
- }
+ bind(ChangesByProjectCache.class).to(SearchingChangeCacheImpl.class);
+ DynamicSet.bind(binder(), GitReferenceUpdatedListener.class)
+ .to(SearchingChangeCacheImpl.class);
}
}
@@ -116,9 +104,11 @@ public class SearchingChangeCacheImpl implements GitReferenceUpdatedListener {
* Additional stored fields are not loaded from the index.
*
* @param project project to read.
- * @return list of known changes; empty if no changes.
+ * @param repo repository for the project to read.
+ * @return Collection of known changes; empty if no changes.
*/
- public List<ChangeData> getChangeData(Project.NameKey project) {
+ @Override
+ public Collection<ChangeData> getChangeDatas(Project.NameKey project, Repository unusedrepo) {
try {
List<CachedChange> cached = cache.get(project);
List<ChangeData> cds = new ArrayList<>(cached.size());
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index 0bcf8fc6df..0b1bf7f3af 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -162,6 +162,12 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
return new ChangeNotes(args, newChange(project, changeId), true, null).load();
}
+ public ChangeNotes create(
+ Project.NameKey project, Change.Id changeId, @Nullable ObjectId metaRevId) {
+ checkArgument(project != null, "project is required");
+ return new ChangeNotes(args, newChange(project, changeId), true, null, metaRevId).load();
+ }
+
public ChangeNotes create(Repository repository, Project.NameKey project, Change.Id changeId) {
checkArgument(project != null, "project is required");
return new ChangeNotes(args, newChange(project, changeId), true, null).load(repository);
diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java
index 79ea9af893..b5f52834db 100644
--- a/java/com/google/gerrit/server/permissions/ChangeControl.java
+++ b/java/com/google/gerrit/server/permissions/ChangeControl.java
@@ -62,7 +62,7 @@ class ChangeControl {
/** Can this user see this change? */
boolean isVisible() {
- if (getChange().isPrivate() && !isPrivateVisible(changeData)) {
+ if (changeData.isPrivateOrThrow() && !isPrivateVisible(changeData)) {
return false;
}
// Does the user have READ permission on the destination?
diff --git a/java/com/google/gerrit/server/permissions/PermissionBackend.java b/java/com/google/gerrit/server/permissions/PermissionBackend.java
index 27c6793f21..d40b1381f3 100644
--- a/java/com/google/gerrit/server/permissions/PermissionBackend.java
+++ b/java/com/google/gerrit/server/permissions/PermissionBackend.java
@@ -269,7 +269,7 @@ public abstract class PermissionBackend {
/** Returns an instance scoped for the change, and its destination ref and project. */
public ForChange change(ChangeData cd) {
try {
- return ref(cd.change().getDest().branch()).change(cd);
+ return ref(cd.branchOrThrow().branch()).change(cd);
} catch (StorageException e) {
return FailedPermissionBackend.change("unavailable", e);
}
diff --git a/java/com/google/gerrit/server/permissions/ProjectControl.java b/java/com/google/gerrit/server/permissions/ProjectControl.java
index 1203049d5e..8100457df0 100644
--- a/java/com/google/gerrit/server/permissions/ProjectControl.java
+++ b/java/com/google/gerrit/server/permissions/ProjectControl.java
@@ -114,7 +114,7 @@ class ProjectControl {
}
ChangeControl controlFor(ChangeData cd) {
- return new ChangeControl(controlForRef(cd.change().getDest()), cd);
+ return new ChangeControl(controlForRef(cd.branchOrThrow()), cd);
}
RefControl controlForRef(BranchNameKey ref) {
@@ -366,7 +366,7 @@ class ProjectControl {
@Override
public ForChange change(ChangeData cd) {
try {
- checkProject(cd.change());
+ checkProject(cd);
return super.change(cd);
} catch (StorageException e) {
return FailedPermissionBackend.change("unavailable", e);
@@ -379,13 +379,21 @@ class ProjectControl {
return super.change(notes);
}
+ private void checkProject(ChangeData cd) {
+ checkProject(cd.project());
+ }
+
private void checkProject(Change change) {
+ checkProject(change.getProject());
+ }
+
+ private void checkProject(Project.NameKey changeProject) {
Project.NameKey project = getProject().getNameKey();
checkArgument(
- project.equals(change.getProject()),
+ project.equals(changeProject),
"expected change in project %s, not %s",
project,
- change.getProject());
+ changeProject);
}
@Override
diff --git a/java/com/google/gerrit/server/permissions/VisibleChangesCache.java b/java/com/google/gerrit/server/permissions/VisibleChangesCache.java
index 8a68a9927a..6bb3204f34 100644
--- a/java/com/google/gerrit/server/permissions/VisibleChangesCache.java
+++ b/java/com/google/gerrit/server/permissions/VisibleChangesCache.java
@@ -14,19 +14,12 @@
package com.google.gerrit.server.permissions;
-import static com.google.common.collect.ImmutableList.toImmutableList;
-
-import com.google.common.collect.ImmutableList;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
-import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.server.git.SearchingChangeCacheImpl;
-import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.notedb.ChangeNotes.Factory.ChangeNotesResult;
+import com.google.gerrit.server.git.ChangesByProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
@@ -36,10 +29,7 @@ import java.util.HashMap;
import java.util.Map;
import org.eclipse.jgit.lib.Repository;
-/**
- * Gets all of the visible by current user changes in the repository that are available in the
- * change index and cache.
- */
+/** Gets all the changes in a repository visible by the current user. */
class VisibleChangesCache {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -47,26 +37,23 @@ class VisibleChangesCache {
VisibleChangesCache create(ProjectControl projectControl, Repository repository);
}
- @Nullable private final SearchingChangeCacheImpl changeCache;
private final ProjectState projectState;
- private final ChangeNotes.Factory changeNotesFactory;
private final PermissionBackend.ForProject permissionBackendForProject;
+ private final ChangesByProjectCache changesByProjectCache;
private final Repository repository;
private Map<Change.Id, BranchNameKey> visibleChanges;
@Inject
VisibleChangesCache(
- @Nullable SearchingChangeCacheImpl changeCache,
PermissionBackend permissionBackend,
- ChangeNotes.Factory changeNotesFactory,
+ ChangesByProjectCache changesByProjectCache,
@Assisted ProjectControl projectControl,
@Assisted Repository repository) {
- this.changeCache = changeCache;
this.projectState = projectControl.getProjectState();
this.permissionBackendForProject =
permissionBackend.user(projectControl.getUser()).project(projectState.getNameKey());
- this.changeNotesFactory = changeNotesFactory;
+ this.changesByProjectCache = changesByProjectCache;
this.repository = repository;
}
@@ -75,21 +62,16 @@ class VisibleChangesCache {
* by looking at the cached visible changes.
*/
public boolean isVisible(Change.Id changeId) throws PermissionBackendException {
- cachedVisibleChanges();
- return visibleChanges.containsKey(changeId);
+ return cachedVisibleChanges().containsKey(changeId);
}
/**
* Returns the visible changes in the repository {@code repo}. If not cached, computes the visible
* changes and caches them.
*/
- public Map<Change.Id, BranchNameKey> cachedVisibleChanges() throws PermissionBackendException {
+ public Map<Change.Id, BranchNameKey> cachedVisibleChanges() {
if (visibleChanges == null) {
- if (changeCache == null) {
- visibleChangesByScan();
- } else {
- visibleChangesBySearch();
- }
+ loadVisibleChanges();
logger.atFinest().log("Visible changes: %s", visibleChanges.keySet());
}
return visibleChanges;
@@ -105,64 +87,21 @@ class VisibleChangesCache {
return cachedVisibleChanges().get(changeId);
}
- private void visibleChangesBySearch() throws PermissionBackendException {
+ private void loadVisibleChanges() {
visibleChanges = new HashMap<>();
if (!projectState.statePermitsRead()) {
return;
}
Project.NameKey project = projectState.getNameKey();
try {
- for (ChangeData cd : changeCache.getChangeData(project)) {
- try {
- permissionBackendForProject.change(cd).check(ChangePermission.READ);
- visibleChanges.put(cd.getId(), cd.change().getDest());
- } catch (AuthException e) {
- // Do nothing.
+ for (ChangeData cd : changesByProjectCache.getChangeDatas(project, repository)) {
+ if (permissionBackendForProject.change(cd).testOrFalse(ChangePermission.READ)) {
+ visibleChanges.put(cd.getId(), cd.branchOrThrow());
}
}
- } catch (StorageException e) {
- logger.atSevere().withCause(e).log(
- "Cannot load changes for project %s, assuming no changes are visible", project);
- }
- }
-
- private void visibleChangesByScan() throws PermissionBackendException {
- visibleChanges = new HashMap<>();
- if (!projectState.statePermitsRead()) {
- return;
- }
- Project.NameKey p = projectState.getNameKey();
- ImmutableList<ChangeNotesResult> changes;
- try {
- changes = changeNotesFactory.scan(repository, p).collect(toImmutableList());
} catch (IOException e) {
logger.atSevere().withCause(e).log(
- "Cannot load changes for project %s, assuming no changes are visible", p);
- return;
- }
-
- for (ChangeNotesResult notesResult : changes) {
- ChangeNotes notes = toNotes(notesResult);
- if (notes != null) {
- visibleChanges.put(notes.getChangeId(), notes.getChange().getDest());
- }
- }
- }
-
- @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;
- }
-
- try {
- permissionBackendForProject.change(r.notes()).check(ChangePermission.READ);
- return r.notes();
- } catch (AuthException e) {
- // Skip.
+ "Cannot load changes for project %s, assuming no changes are visible", project);
}
- return null;
}
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index 3b47aae56f..7d7b17bd02 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -40,6 +40,7 @@ import com.google.common.primitives.Ints;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AttentionSetUpdate;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.ChangeMessage;
import com.google.gerrit.entities.Comment;
@@ -240,6 +241,19 @@ public class ChangeData {
return assistedFactory.create(project, id, null, null);
}
+ public ChangeData create(Project.NameKey project, Change.Id id, ObjectId metaRevision) {
+ ChangeData cd = assistedFactory.create(project, id, null, null);
+ cd.setMetaRevision(metaRevision);
+ return cd;
+ }
+
+ public ChangeData createNonPrivate(BranchNameKey branch, Change.Id id, ObjectId metaRevision) {
+ ChangeData cd = create(branch.project(), id, metaRevision);
+ cd.branch = branch.branch();
+ cd.isPrivate = false;
+ return cd;
+ }
+
public ChangeData create(Change change) {
return assistedFactory.create(change.getProject(), change.getId(), change, null);
}
@@ -332,7 +346,10 @@ public class ChangeData {
private List<ChangeMessage> messages;
private Optional<ChangedLines> changedLines;
private SubmitTypeRecord submitTypeRecord;
+ private String branch;
+ private Boolean isPrivate;
private Boolean mergeable;
+ private ObjectId metaRevision;
private Set<String> hashtags;
/**
* Map from {@link com.google.gerrit.entities.Account.Id} to the tip of the edit ref for this
@@ -532,6 +549,55 @@ public class ChangeData {
return project;
}
+ public BranchNameKey branchOrThrow() {
+ if (change == null) {
+ if (branch != null) {
+ return BranchNameKey.create(project, branch);
+ }
+ throwIfNotLazyLoad("branch");
+ change();
+ }
+ return change.getDest();
+ }
+
+ public boolean isPrivateOrThrow() {
+ if (change == null) {
+ if (isPrivate != null) {
+ return isPrivate;
+ }
+ throwIfNotLazyLoad("isPrivate");
+ change();
+ }
+ return change.isPrivate();
+ }
+
+ public ChangeData setMetaRevision(ObjectId metaRevision) {
+ this.metaRevision = metaRevision;
+ return this;
+ }
+
+ public ObjectId metaRevisionOrThrow() {
+ if (notes == null) {
+ if (metaRevision != null) {
+ return metaRevision;
+ }
+ if (refStates != null) {
+ Set<RefState> refs = refStates.get(project);
+ if (refs != null) {
+ String metaRef = RefNames.changeMetaRef(getId());
+ for (RefState r : refs) {
+ if (r.ref().equals(metaRef)) {
+ return r.id();
+ }
+ }
+ }
+ }
+ throwIfNotLazyLoad("metaRevision");
+ notes();
+ }
+ return notes.getRevision();
+ }
+
boolean fastIsVisibleTo(CurrentUser user) {
return visibleTo == user;
}
@@ -542,7 +608,7 @@ public class ChangeData {
public Change change() {
if (change == null && lazyload()) {
- reloadChange();
+ loadChange();
}
return change;
}
@@ -552,12 +618,18 @@ public class ChangeData {
}
public Change reloadChange() {
+ metaRevision = null;
+ return loadChange();
+ }
+
+ private Change loadChange() {
try {
- notes = notesFactory.createChecked(project, legacyId);
+ notes = notesFactory.createChecked(project, legacyId, metaRevision);
} catch (NoSuchChangeException e) {
throw new StorageException("Unable to load change " + legacyId, e);
}
change = notes.getChange();
+ metaRevision = null;
setPatchSets(null);
return change;
}
@@ -575,7 +647,8 @@ public class ChangeData {
if (!lazyload()) {
throw new StorageException("ChangeNotes not available, lazyLoad = false");
}
- notes = notesFactory.create(project(), legacyId);
+ notes = notesFactory.create(project(), legacyId, metaRevision);
+ change = notes.getChange();
}
return notes;
}
@@ -796,11 +869,7 @@ public class ChangeData {
public ReviewerSet reviewers() {
if (reviewers == null) {
- if (!lazyload()) {
- // We are not allowed to load values from NoteDb. Reviewers were not populated with values
- // from the index. However, we need these values for permission checks.
- throw new IllegalStateException("reviewers not populated");
- }
+ throwIfNotLazyLoad("reviewers");
reviewers = approvalsUtil.getReviewers(notes());
}
return reviewers;
@@ -1352,6 +1421,14 @@ public class ChangeData {
this.refStatePatterns = ImmutableList.copyOf(refStatePatterns);
}
+ private void throwIfNotLazyLoad(String field) {
+ if (!lazyload()) {
+ // We are not allowed to load values from NoteDb. 'field' was not populated, however,
+ // we need this value for permission checks.
+ throw new IllegalStateException("'" + field + "' field not populated");
+ }
+ }
+
@AutoValue
abstract static class ReviewedByEvent {
private static ReviewedByEvent create(ChangeMessage msg) {
diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java
index 49edd4c4d4..966a9c1ead 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -76,10 +76,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.experiments.ConfigExperimentFeatures.ConfigExperimentFeaturesModule;
+import com.google.gerrit.server.git.ChangesByProjectCache;
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.SearchingChangeCacheImplModule;
import com.google.gerrit.server.git.WorkQueue;
import com.google.gerrit.server.group.testing.TestGroupBackend;
import com.google.gerrit.server.index.account.AccountSchemaDefinitions;
@@ -192,7 +192,7 @@ public class InMemoryModule extends FactoryModule {
factory(PluginUser.Factory.class);
install(new PluginApiModule());
install(new DefaultPermissionBackendModule());
- install(new SearchingChangeCacheImplModule());
+ install(new ChangesByProjectCache.Module(ChangesByProjectCache.UseIndex.TRUE, cfg));
factory(GarbageCollection.Factory.class);
install(new AuditModule());
install(new SubscriptionGraphModule());