Cache the OWNERS file's parsed content
The OWNERS file is rarely modified however it is being read and parsed
for every single change that gets evaluated. Introduce the cache (called
`owners-path_owners_entries`) that holds the PathOwnersEntry value for a
project, branch and path (typically `refs/meta/config`, development
branch and path). The path is especially important for projects with
deep directories structure (mono-repos) and cache size should be
carefully adjusted to adhere to it (1 dir level = 1 cache entry).
It gets invalidated when project's ref is updated.
As a result it reduces 2-3 times the configuration load latency which is
reflected in performance of prolog and submit requirements.
There are several constraints that influenced the implementation
* the `PathOwners` is a common class used by both owners and
owners-autoassign plugins
* the `PathOwners`, in the owners plugin prolog's engine, is
instantiated by the constructor call and injection is not possible
there
The config.md was updated with the cache configuration.
Bug: Issue 16830
Change-Id: Ic7d61de07f3ec5169f0cf7c4ae8266351543e108
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AutoassignModule.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AutoassignModule.java
index 80a03e0..5e6ad99 100644
--- a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AutoassignModule.java
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AutoassignModule.java
@@ -49,6 +49,7 @@
@Override
protected void configure() {
+ install(PathOwnersEntriesCache.module());
bind(ReviewerManager.class)
.to(config.isAsyncReviewers() ? AsyncReviewerManager.class : SyncReviewerManager.class);
DynamicSet.bind(binder(), GitReferenceUpdatedListener.class).to(GitRefListener.class);
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
index faf5e42..d6fd574 100644
--- a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
@@ -89,6 +89,8 @@
private final AutoAssignConfig cfg;
+ private final PathOwnersEntriesCache cache;
+
@Inject
public GitRefListener(
GerritApi api,
@@ -100,7 +102,8 @@
OneOffRequestContext oneOffReqCtx,
Provider<CurrentUser> currentUserProvider,
ChangeNotes.Factory notesFactory,
- AutoAssignConfig cfg) {
+ AutoAssignConfig cfg,
+ PathOwnersEntriesCache cache) {
this.api = api;
this.patchListCache = patchListCache;
this.projectCache = projectCache;
@@ -111,6 +114,7 @@
this.currentUserProvider = currentUserProvider;
this.notesFactory = notesFactory;
this.cfg = cfg;
+ this.cache = cache;
}
@Override
@@ -235,7 +239,9 @@
parentProjectsNameKeys,
cfg.isBranchDisabled(change.branch) ? Optional.empty() : Optional.of(change.branch),
patchList,
- cfg.expandGroups());
+ cfg.expandGroups(),
+ projectNameKey.get(),
+ cache);
Set<Account.Id> allReviewers = Sets.newHashSet();
allReviewers.addAll(owners.get().values());
allReviewers.addAll(owners.getReviewers().values());
diff --git a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerIT.java b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerIT.java
index e5be7b5..cb34173 100644
--- a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerIT.java
+++ b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerIT.java
@@ -37,12 +37,9 @@
import com.google.inject.Inject;
import com.google.inject.Module;
import com.googlesource.gerrit.owners.api.OwnersApiModule;
-import com.googlesource.gerrit.owners.common.AutoassignConfigModule;
import java.util.HashMap;
import java.util.Map;
import java.util.stream.StreamSupport;
-import com.googlesource.gerrit.owners.common.ReviewerManager;
-
import org.eclipse.jgit.transport.ReceiveCommand.Type;
import org.junit.Test;
@@ -66,6 +63,7 @@
public static class TestModule extends AbstractModule {
@Override
protected void configure() {
+ install(PathOwnersEntriesCache.module());
DynamicSet.bind(binder(), GitReferenceUpdatedListener.class).to(GitRefListenerTest.class);
install(new AutoassignConfigModule());
}
diff --git a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerTest.java b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerTest.java
index 9cc6e96..b651572 100644
--- a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerTest.java
+++ b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerTest.java
@@ -27,10 +27,6 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
-import com.googlesource.gerrit.owners.common.Accounts;
-import com.googlesource.gerrit.owners.common.AutoAssignConfig;
-import com.googlesource.gerrit.owners.common.GitRefListener;
-import com.googlesource.gerrit.owners.common.ReviewerManager;
import org.eclipse.jgit.lib.Repository;
import org.junit.Ignore;
@@ -50,7 +46,8 @@
OneOffRequestContext oneOffReqCtx,
Provider<CurrentUser> currentUserProvider,
ChangeNotes.Factory notesFactory,
- AutoAssignConfig cfg) {
+ AutoAssignConfig cfg,
+ PathOwnersEntriesCache cache) {
super(
api,
patchListCache,
@@ -61,7 +58,8 @@
oneOffReqCtx,
currentUserProvider,
notesFactory,
- cfg);
+ cfg,
+ cache);
}
@Override
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
index 8269ad6..df398e3 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
@@ -43,6 +43,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
+import java.util.concurrent.ExecutionException;
import java.util.stream.Collectors;
import org.eclipse.jgit.lib.Repository;
import org.slf4j.Logger;
@@ -99,7 +100,9 @@
List<Project.NameKey> parentProjectsNames,
Optional<String> branchWhenEnabled,
Map<String, FileDiffOutput> fileDiffMap,
- boolean expandGroups) {
+ boolean expandGroups,
+ String project,
+ PathOwnersEntriesCache cache) {
this(
accounts,
repositoryManager,
@@ -107,7 +110,9 @@
parentProjectsNames,
branchWhenEnabled,
getModifiedPaths(fileDiffMap),
- expandGroups);
+ expandGroups,
+ project,
+ cache);
}
public PathOwners(
@@ -117,7 +122,9 @@
List<Project.NameKey> parentProjectsNames,
Optional<String> branchWhenEnabled,
DiffSummary diffSummary,
- boolean expandGroups) {
+ boolean expandGroups,
+ String project,
+ PathOwnersEntriesCache cache) {
this(
accounts,
repositoryManager,
@@ -125,7 +132,9 @@
parentProjectsNames,
branchWhenEnabled,
ImmutableSet.copyOf(diffSummary.getPaths()),
- expandGroups);
+ expandGroups,
+ project,
+ cache);
}
public PathOwners(
@@ -135,7 +144,9 @@
List<Project.NameKey> parentProjectsNames,
Optional<String> branchWhenEnabled,
Set<String> modifiedPaths,
- boolean expandGroups) {
+ boolean expandGroups,
+ String project,
+ PathOwnersEntriesCache cache) {
this.repositoryManager = repositoryManager;
this.repository = repository;
this.parentProjectsNames = parentProjectsNames;
@@ -144,7 +155,10 @@
this.accounts = accounts;
this.expandGroups = expandGroups;
- OwnersMap map = branchWhenEnabled.map(branch -> fetchOwners(branch)).orElse(new OwnersMap());
+ OwnersMap map =
+ branchWhenEnabled
+ .map(branch -> fetchOwners(project, branch, cache))
+ .orElse(new OwnersMap());
owners = Multimaps.unmodifiableSetMultimap(map.getPathOwners());
reviewers = Multimaps.unmodifiableSetMultimap(map.getPathReviewers());
matchers = map.getMatchers();
@@ -195,21 +209,29 @@
*
* @return A structure containing matchers paths to owners
*/
- private OwnersMap fetchOwners(String branch) {
+ private OwnersMap fetchOwners(String project, String branch, PathOwnersEntriesCache cache) {
OwnersMap ownersMap = new OwnersMap();
try {
// Using a `map` would have needed a try/catch inside the lamba, resulting in more code
List<PathOwnersEntry> parentsPathOwnersEntries =
- getPathOwnersEntries(parentProjectsNames, RefNames.REFS_CONFIG);
- PathOwnersEntry projectEntry = getPathOwnersEntry(repository, RefNames.REFS_CONFIG);
- PathOwnersEntry rootEntry = getPathOwnersEntry(repository, branch);
+ getPathOwnersEntries(parentProjectsNames, RefNames.REFS_CONFIG, cache);
+ PathOwnersEntry projectEntry =
+ getPathOwnersEntry(project, repository, RefNames.REFS_CONFIG, cache);
+ PathOwnersEntry rootEntry = getPathOwnersEntry(project, repository, branch, cache);
Map<String, PathOwnersEntry> entries = new HashMap<>();
PathOwnersEntry currentEntry = null;
for (String path : modifiedPaths) {
currentEntry =
resolvePathEntry(
- path, branch, projectEntry, parentsPathOwnersEntries, rootEntry, entries);
+ project,
+ path,
+ branch,
+ projectEntry,
+ parentsPathOwnersEntries,
+ rootEntry,
+ entries,
+ cache);
// add owners and reviewers to file for matcher predicates
ownersMap.addFileOwners(path, currentEntry.getOwners());
@@ -239,38 +261,47 @@
}
ownersMap.setLabel(Optional.ofNullable(currentEntry).flatMap(PathOwnersEntry::getLabel));
return ownersMap;
- } catch (IOException e) {
+ } catch (IOException | ExecutionException e) {
log.warn("Invalid OWNERS file", e);
return ownersMap;
}
}
private List<PathOwnersEntry> getPathOwnersEntries(
- List<Project.NameKey> projectNames, String branch) throws IOException {
+ List<Project.NameKey> projectNames, String branch, PathOwnersEntriesCache cache)
+ throws IOException, ExecutionException {
ImmutableList.Builder<PathOwnersEntry> pathOwnersEntries = ImmutableList.builder();
for (Project.NameKey projectName : projectNames) {
try (Repository repo = repositoryManager.openRepository(projectName)) {
- pathOwnersEntries = pathOwnersEntries.add(getPathOwnersEntry(repo, branch));
+ pathOwnersEntries =
+ pathOwnersEntries.add(getPathOwnersEntry(projectName.get(), repo, branch, cache));
}
}
return pathOwnersEntries.build();
}
- private PathOwnersEntry getPathOwnersEntry(Repository repo, String branch) throws IOException {
+ private PathOwnersEntry getPathOwnersEntry(
+ String project, Repository repo, String branch, PathOwnersEntriesCache cache)
+ throws ExecutionException {
String rootPath = "OWNERS";
- return getOwnersConfig(repo, rootPath, branch)
- .map(
- conf ->
- new PathOwnersEntry(
- rootPath,
- conf,
- accounts,
- Optional.empty(),
- Collections.emptySet(),
- Collections.emptySet(),
- Collections.emptySet(),
- Collections.emptySet()))
- .orElse(new PathOwnersEntry());
+ return cache.get(
+ project,
+ branch,
+ rootPath,
+ () ->
+ getOwnersConfig(repo, rootPath, branch)
+ .map(
+ conf ->
+ new PathOwnersEntry(
+ rootPath,
+ conf,
+ accounts,
+ Optional.empty(),
+ Collections.emptySet(),
+ Collections.emptySet(),
+ Collections.emptySet(),
+ Collections.emptySet()))
+ .orElse(new PathOwnersEntry()));
}
private void processMatcherPerPath(
@@ -318,13 +349,15 @@
}
private PathOwnersEntry resolvePathEntry(
+ String project,
String path,
String branch,
PathOwnersEntry projectEntry,
List<PathOwnersEntry> parentsPathOwnersEntries,
PathOwnersEntry rootEntry,
- Map<String, PathOwnersEntry> entries)
- throws IOException {
+ Map<String, PathOwnersEntry> entries,
+ PathOwnersEntriesCache cache)
+ throws ExecutionException {
String[] parts = path.split("/");
PathOwnersEntry currentEntry = rootEntry;
StringBuilder builder = new StringBuilder();
@@ -349,25 +382,33 @@
currentEntry = entries.get(partial);
} else {
String ownersPath = partial + "OWNERS";
- Optional<OwnersConfig> conf = getOwnersConfig(repository, ownersPath, branch);
- Optional<LabelDefinition> label = currentEntry.getLabel();
- final Set<Id> owners = currentEntry.getOwners();
- final Set<Id> reviewers = currentEntry.getReviewers();
- Collection<Matcher> inheritedMatchers = currentEntry.getMatchers().values();
- Set<String> groupOwners = currentEntry.getGroupOwners();
+ PathOwnersEntry pathFallbackEntry = currentEntry;
currentEntry =
- conf.map(
- c ->
- new PathOwnersEntry(
- ownersPath,
- c,
- accounts,
- label,
- owners,
- reviewers,
- inheritedMatchers,
- groupOwners))
- .orElse(currentEntry);
+ cache.get(
+ project,
+ branch,
+ ownersPath,
+ () ->
+ getOwnersConfig(repository, ownersPath, branch)
+ .map(
+ c -> {
+ Optional<LabelDefinition> label = pathFallbackEntry.getLabel();
+ final Set<Id> owners = pathFallbackEntry.getOwners();
+ final Set<Id> reviewers = pathFallbackEntry.getReviewers();
+ Collection<Matcher> inheritedMatchers =
+ pathFallbackEntry.getMatchers().values();
+ Set<String> groupOwners = pathFallbackEntry.getGroupOwners();
+ return new PathOwnersEntry(
+ ownersPath,
+ c,
+ accounts,
+ label,
+ owners,
+ reviewers,
+ inheritedMatchers,
+ groupOwners);
+ })
+ .orElse(pathFallbackEntry));
entries.put(partial, currentEntry);
}
}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java
new file mode 100644
index 0000000..07be702
--- /dev/null
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java
@@ -0,0 +1,141 @@
+// 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.googlesource.gerrit.owners.common;
+
+import com.google.common.cache.RemovalNotification;
+import com.google.gerrit.extensions.annotations.PluginName;
+import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.server.cache.CacheModule;
+import com.google.gerrit.server.cache.CacheRemovalListener;
+import com.google.inject.Inject;
+import com.google.inject.Module;
+import com.google.inject.Singleton;
+import java.util.Objects;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+
+public interface PathOwnersEntriesCache {
+ String CACHE_NAME = "path_owners_entries";
+
+ static Module module() {
+ return new CacheModule() {
+ @Override
+ protected void configure() {
+ cache(CACHE_NAME, Key.class, PathOwnersEntry.class);
+ bind(PathOwnersEntriesCache.class).to(PathOwnersEntriesCacheImpl.class);
+ DynamicSet.bind(binder(), GitReferenceUpdatedListener.class)
+ .to(OwnersRefUpdateListener.class);
+ DynamicSet.bind(binder(), CacheRemovalListener.class).to(OwnersCacheRemovalListener.class);
+ }
+ };
+ }
+
+ PathOwnersEntry get(String project, String branch, String path, Callable<PathOwnersEntry> loader)
+ throws ExecutionException;
+
+ void invalidate(String project, String branch);
+
+ void invalidateIndexKey(Key key);
+
+ class Key {
+ final String project;
+ final String branch;
+ final String path;
+
+ Key(String project, String branch, String path) {
+ this.project = project;
+ this.branch = branch;
+ this.path = path;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(branch, path, project);
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+
+ if (obj == null) {
+ return false;
+ }
+
+ if (getClass() != obj.getClass()) {
+ return false;
+ }
+
+ Key other = (Key) obj;
+ return Objects.equals(branch, other.branch)
+ && Objects.equals(path, other.path)
+ && Objects.equals(project, other.project);
+ }
+
+ @Override
+ public String toString() {
+ StringBuilder builder = new StringBuilder();
+ builder.append("Key [project=");
+ builder.append(project);
+ builder.append(", branch=");
+ builder.append(branch);
+ builder.append(", path=");
+ builder.append(path);
+ builder.append("]");
+ return builder.toString();
+ }
+ }
+
+ @Singleton
+ class OwnersRefUpdateListener implements GitReferenceUpdatedListener {
+ private final PathOwnersEntriesCache cache;
+
+ @Inject
+ OwnersRefUpdateListener(PathOwnersEntriesCache cache) {
+ this.cache = cache;
+ }
+
+ @Override
+ public void onGitReferenceUpdated(Event event) {
+ cache.invalidate(event.getProjectName(), event.getRefName());
+ }
+ }
+
+ @Singleton
+ class OwnersCacheRemovalListener implements CacheRemovalListener<Key, PathOwnersEntry> {
+ private final PathOwnersEntriesCache cache;
+ private final String cacheName;
+
+ @Inject
+ OwnersCacheRemovalListener(@PluginName String pluginName, PathOwnersEntriesCache cache) {
+ this.cache = cache;
+ this.cacheName = String.format("%s.%s", pluginName, CACHE_NAME);
+ }
+
+ @Override
+ public void onRemoval(
+ String pluginName,
+ String cacheName,
+ RemovalNotification<Key, PathOwnersEntry> notification) {
+ if (!this.cacheName.equals(cacheName)) {
+ return;
+ }
+
+ cache.invalidateIndexKey(notification.getKey());
+ }
+ }
+}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheImpl.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheImpl.java
new file mode 100644
index 0000000..74f9585
--- /dev/null
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheImpl.java
@@ -0,0 +1,92 @@
+// 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.googlesource.gerrit.owners.common;
+
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import com.google.common.collect.HashMultimap;
+import com.google.common.collect.Multimap;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import com.google.inject.name.Named;
+import java.time.Duration;
+import java.util.Collection;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+
+@Singleton
+class PathOwnersEntriesCacheImpl implements PathOwnersEntriesCache {
+
+ private final Cache<Key, PathOwnersEntry> cache;
+ private final Multimap<String, Key> keysIndex;
+ private final LoadingCache<String, Object> keyLocks;
+
+ @Inject
+ PathOwnersEntriesCacheImpl(@Named(CACHE_NAME) Cache<Key, PathOwnersEntry> cache) {
+ this.cache = cache;
+ this.keysIndex = HashMultimap.create();
+ this.keyLocks =
+ CacheBuilder.newBuilder()
+ .expireAfterAccess(Duration.ofMinutes(10L))
+ .build(CacheLoader.from(Object::new));
+ }
+
+ @Override
+ public PathOwnersEntry get(
+ String project, String branch, String path, Callable<PathOwnersEntry> loader)
+ throws ExecutionException {
+ Key key = new Key(project, branch, path);
+ return cache.get(
+ key,
+ () -> {
+ PathOwnersEntry entry = loader.call();
+ String indexKey = indexKey(project, branch);
+ synchronized (keyLocks.getUnchecked(indexKey)) {
+ keysIndex.put(indexKey, key);
+ }
+ return entry;
+ });
+ }
+
+ @Override
+ public void invalidate(String project, String branch) {
+ String indexKey = indexKey(project, branch);
+ Collection<Key> keysToInvalidate;
+
+ synchronized (keyLocks.getUnchecked(indexKey)) {
+ keysToInvalidate = keysIndex.removeAll(indexKey);
+ }
+
+ keysToInvalidate.forEach(cache::invalidate);
+ }
+
+ @Override
+ public void invalidateIndexKey(Key key) {
+ String indexKey = indexKey(key.project, key.branch);
+
+ synchronized (keyLocks.getUnchecked(indexKey)) {
+ Collection<Key> values = keysIndex.asMap().get(indexKey);
+ if (values != null) {
+ values.remove(key);
+ }
+ }
+ }
+
+ private String indexKey(String project, String branch) {
+ return new StringBuilder(project).append('@').append(branch).toString();
+ }
+}
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheMock.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheMock.java
new file mode 100644
index 0000000..1eac62c
--- /dev/null
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheMock.java
@@ -0,0 +1,43 @@
+// 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.googlesource.gerrit.owners.common;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import org.junit.Ignore;
+
+/** This is a test implementation that doesn't cache anything but calls loader instead. */
+@Ignore
+public class PathOwnersEntriesCacheMock implements PathOwnersEntriesCache {
+ int hit = 0;
+
+ @Override
+ public void invalidate(String project, String branch) {}
+
+ @Override
+ public void invalidateIndexKey(Key key) {}
+
+ @Override
+ public PathOwnersEntry get(
+ String project, String branch, String path, Callable<PathOwnersEntry> loader)
+ throws ExecutionException {
+ try {
+ hit++;
+ return loader.call();
+ } catch (Exception e) {
+ throw new ExecutionException(e);
+ }
+ }
+}
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
index 710a2b7..2e23f33 100644
--- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
@@ -21,7 +21,9 @@
import static org.easymock.EasyMock.eq;
import static org.easymock.EasyMock.expect;
import static org.easymock.EasyMock.expectLastCall;
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
import static org.powermock.api.easymock.PowerMock.replayAll;
import com.google.gerrit.entities.Account;
@@ -50,6 +52,8 @@
private static final boolean DO_NOT_EXPAND_GROUPS = false;
private static final String EXPECTED_LABEL = "expected-label";
private static final String A_LABEL = "a-label";
+ private static PathOwnersEntriesCache CACHE_MOCK = new PathOwnersEntriesCacheMock();
+
public static final String CLASSIC_FILE_TXT = "classic/file.txt";
public static final Project.NameKey parentRepository1NameKey =
Project.NameKey.parse("parentRepository1");
@@ -74,7 +78,9 @@
emptyList(),
branch,
Set.of(CLASSIC_FILE_TXT),
- EXPAND_GROUPS);
+ EXPAND_GROUPS,
+ "foo",
+ CACHE_MOCK);
Set<Account.Id> ownersSet = owners.get().get(CLASSIC_OWNERS);
assertEquals(2, ownersSet.size());
assertTrue(ownersSet.contains(USER_A_ID));
@@ -94,7 +100,9 @@
emptyList(),
branch,
Set.of(CLASSIC_FILE_TXT),
- DO_NOT_EXPAND_GROUPS);
+ DO_NOT_EXPAND_GROUPS,
+ "foo",
+ CACHE_MOCK);
Set<String> ownersSet = owners.getFileGroupOwners().get(CLASSIC_FILE_TXT);
assertEquals(2, ownersSet.size());
assertTrue(ownersSet.contains(USER_A));
@@ -114,7 +122,9 @@
emptyList(),
Optional.empty(),
Set.of(CLASSIC_FILE_TXT),
- EXPAND_GROUPS);
+ EXPAND_GROUPS,
+ "foo",
+ CACHE_MOCK);
Set<Account.Id> ownersSet = owners.get().get(CLASSIC_OWNERS);
assertEquals(0, ownersSet.size());
}
@@ -137,7 +147,9 @@
emptyList(),
branch,
Set.of("classic/file.txt"),
- EXPAND_GROUPS);
+ EXPAND_GROUPS,
+ "foo",
+ CACHE_MOCK);
Set<Account.Id> ownersSet2 = owners2.get().get(CLASSIC_OWNERS);
// in this case we are inheriting the acct3 from /OWNERS
@@ -174,7 +186,9 @@
emptyList(),
branch,
Set.of(fileName),
- EXPAND_GROUPS);
+ EXPAND_GROUPS,
+ "foo",
+ CACHE_MOCK);
Map<String, Set<Account.Id>> fileOwners = owners.getFileOwners();
assertEquals(1, fileOwners.size());
@@ -217,7 +231,9 @@
Arrays.asList(parentRepository1NameKey),
branch,
Set.of(fileName),
- EXPAND_GROUPS);
+ EXPAND_GROUPS,
+ "foo",
+ CACHE_MOCK);
Map<String, Set<Account.Id>> fileOwners = owners.getFileOwners();
assertEquals(fileOwners.size(), 1);
@@ -264,7 +280,9 @@
Arrays.asList(parentRepository1NameKey, parentRepository2NameKey),
branch,
Set.of(sqlFileName, javaFileName),
- EXPAND_GROUPS);
+ EXPAND_GROUPS,
+ "foo",
+ CACHE_MOCK);
Map<String, Set<Account.Id>> fileOwners = owners.getFileOwners();
assertEquals(fileOwners.size(), 2);
@@ -306,7 +324,9 @@
emptyList(),
branch,
Set.of("dir/subdir/file.txt"),
- EXPAND_GROUPS);
+ EXPAND_GROUPS,
+ "foo",
+ CACHE_MOCK);
Set<Account.Id> ownersSet = owners.get().get("dir/subdir/OWNERS");
assertEquals(3, ownersSet.size());
@@ -360,6 +380,48 @@
assertTrue(owners.contains(USER_C_EMAIL_COM));
}
+ @Test
+ public void testPathOwnersEntriesCacheIsCalled() throws Exception {
+ expectConfig("OWNERS", "master", createConfig(true, Optional.of(EXPECTED_LABEL), owners()));
+ expectConfig(
+ "OWNERS",
+ RefNames.REFS_CONFIG,
+ repository,
+ createConfig(true, Optional.of("foo"), owners()));
+ expectConfig("dir/OWNERS", createConfig(true, Optional.of(A_LABEL), owners(USER_B_EMAIL_COM)));
+ expectConfig(
+ "dir/subdir/OWNERS",
+ createConfig(true, Optional.of(EXPECTED_LABEL), owners(USER_A_EMAIL_COM)));
+ expectConfig(
+ "OWNERS",
+ RefNames.REFS_CONFIG,
+ parentRepository1,
+ createConfig(true, Optional.of("bar"), owners()));
+
+ mockParentRepository(parentRepository1NameKey, parentRepository1);
+ replayAll();
+
+ PathOwnersEntriesCacheMock cacheMock = new PathOwnersEntriesCacheMock();
+ PathOwners owners =
+ new PathOwners(
+ accounts,
+ repositoryManager,
+ repository,
+ Arrays.asList(parentRepository1NameKey),
+ branch,
+ Set.of("dir/subdir/file.txt"),
+ EXPAND_GROUPS,
+ "foo",
+ cacheMock);
+
+ assertThat(owners.getFileOwners()).isNotEmpty();
+ int expectedCacheCalls =
+ 1 /* for refs/meta/config/OWNERS */
+ + 3 /* for each parent directory of 'file.txt' */
+ + 1 /* for parent's refs/meta/config/OWNERS */;
+ assertThat(cacheMock.hit).isEqualTo(expectedCacheCalls);
+ }
+
private void mockOwners(String... owners) throws IOException {
expectNoConfig("OWNERS");
expectConfig(CLASSIC_OWNERS, createConfig(false, owners(owners)));
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java
index 8ae411e..ac011b0 100644
--- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java
@@ -158,7 +158,9 @@
"project/bfile.txt", // no matching
"projectalfa", // matches PartialRegex
"project/file.sql"), // only .sql matching b,c
- EXPAND_GROUPS);
+ EXPAND_GROUPS,
+ "foo",
+ new PathOwnersEntriesCacheMock());
// assertions on classic owners
Set<Account.Id> ownersSet = owners.get().get("project/OWNERS");
@@ -266,7 +268,9 @@
emptyList(),
branch,
Set.of("project/file.sql", "another.txt"),
- EXPAND_GROUPS);
+ EXPAND_GROUPS,
+ "foo",
+ new PathOwnersEntriesCacheMock());
Set<String> ownedFiles = owners.getFileOwners().keySet();
assertThat(ownedFiles).containsExactly("project/file.sql");
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnerPredicateProvider.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnerPredicateProvider.java
index 119f1bd..d15da8e 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnerPredicateProvider.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnerPredicateProvider.java
@@ -21,14 +21,19 @@
import com.google.gerrit.server.rules.PredicateProvider;
import com.google.inject.Inject;
import com.googlesource.gerrit.owners.common.Accounts;
+import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache;
import com.googlesource.gerrit.owners.common.PluginSettings;
/** Gerrit OWNERS Prolog Predicate Provider. */
@Listen
public class OwnerPredicateProvider implements PredicateProvider {
@Inject
- public OwnerPredicateProvider(Accounts accounts, PluginSettings config, OwnersMetrics metrics) {
- OwnersStoredValues.initialize(accounts, config, metrics);
+ public OwnerPredicateProvider(
+ Accounts accounts,
+ PluginSettings config,
+ PathOwnersEntriesCache cache,
+ OwnersMetrics metrics) {
+ OwnersStoredValues.initialize(accounts, config, cache, metrics);
}
@Override
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersModule.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersModule.java
index bd2f074..7d2c4d5 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersModule.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersModule.java
@@ -20,6 +20,7 @@
import com.google.gerrit.server.rules.PredicateProvider;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
+import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache;
import com.googlesource.gerrit.owners.common.PluginSettings;
public class OwnersModule extends AbstractModule {
@@ -34,6 +35,7 @@
@Override
protected void configure() {
+ install(PathOwnersEntriesCache.module());
DynamicSet.bind(binder(), PredicateProvider.class)
.to(OwnerPredicateProvider.class)
.asEagerSingleton();
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
index fb1de16..02ed2f8 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
@@ -26,6 +26,7 @@
import com.googlecode.prolog_cafe.lang.Prolog;
import com.googlesource.gerrit.owners.common.Accounts;
import com.googlesource.gerrit.owners.common.PathOwners;
+import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache;
import com.googlesource.gerrit.owners.common.PluginSettings;
import java.util.Arrays;
import java.util.Collections;
@@ -43,7 +44,10 @@
public static StoredValue<PathOwners> PATH_OWNERS;
public static synchronized void initialize(
- Accounts accounts, PluginSettings settings, OwnersMetrics metrics) {
+ Accounts accounts,
+ PluginSettings settings,
+ PathOwnersEntriesCache cache,
+ OwnersMetrics metrics) {
if (PATH_OWNERS != null) {
return;
}
@@ -72,7 +76,9 @@
maybeParentProjectNameKey,
settings.isBranchDisabled(branch) ? Optional.empty() : Optional.of(branch),
patchList,
- settings.expandGroups());
+ settings.expandGroups(),
+ projectState.getName(),
+ cache);
}
}
};
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java
index a0339de..97d8098 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java
@@ -48,6 +48,7 @@
import com.googlesource.gerrit.owners.common.Accounts;
import com.googlesource.gerrit.owners.common.LabelDefinition;
import com.googlesource.gerrit.owners.common.PathOwners;
+import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache;
import com.googlesource.gerrit.owners.common.PluginSettings;
import java.io.IOException;
import java.util.Arrays;
@@ -83,6 +84,7 @@
private final GitRepositoryManager repoManager;
private final DiffOperations diffOperations;
private final ApprovalsUtil approvalsUtil;
+ private final PathOwnersEntriesCache cache;
@Inject
OwnersSubmitRequirement(
@@ -92,7 +94,8 @@
Accounts accounts,
GitRepositoryManager repoManager,
DiffOperations diffOperations,
- ApprovalsUtil approvalsUtil) {
+ ApprovalsUtil approvalsUtil,
+ PathOwnersEntriesCache cache) {
this.metrics = metrics;
this.pluginSettings = pluginSettings;
this.projectCache = projectCache;
@@ -100,6 +103,7 @@
this.repoManager = repoManager;
this.diffOperations = diffOperations;
this.approvalsUtil = approvalsUtil;
+ this.cache = cache;
}
@Override
@@ -211,7 +215,9 @@
parents,
pluginSettings.isBranchDisabled(branch) ? Optional.empty() : Optional.of(branch),
getDiff(nameKey, cd.currentPatchSet().commitId()),
- pluginSettings.expandGroups());
+ pluginSettings.expandGroups(),
+ nameKey.get(),
+ cache);
return pathOwners;
}
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
index 475f1cd..d5d403b 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
@@ -40,6 +40,7 @@
import com.google.inject.Singleton;
import com.googlesource.gerrit.owners.common.Accounts;
import com.googlesource.gerrit.owners.common.PathOwners;
+import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache;
import com.googlesource.gerrit.owners.common.PluginSettings;
import com.googlesource.gerrit.owners.entities.FilesOwnersResponse;
import com.googlesource.gerrit.owners.entities.GroupOwner;
@@ -62,6 +63,7 @@
private final PluginSettings pluginSettings;
private final GerritApi gerritApi;
private final ChangeData.Factory changeDataFactory;
+ private final PathOwnersEntriesCache cache;
@Inject
GetFilesOwners(
@@ -72,7 +74,8 @@
GitRepositoryManager repositoryManager,
PluginSettings pluginSettings,
GerritApi gerritApi,
- ChangeData.Factory changeDataFactory) {
+ ChangeData.Factory changeDataFactory,
+ PathOwnersEntriesCache cache) {
this.patchListCache = patchListCache;
this.accounts = accounts;
this.accountCache = accountCache;
@@ -81,6 +84,7 @@
this.pluginSettings = pluginSettings;
this.gerritApi = gerritApi;
this.changeDataFactory = changeDataFactory;
+ this.cache = cache;
}
@Override
@@ -90,14 +94,15 @@
Change change = revision.getChange();
int id = revision.getChangeResource().getChange().getChangeId();
+ Project.NameKey project = change.getProject();
List<Project.NameKey> maybeParentProjectNameKey =
projectCache
- .get(change.getProject())
+ .get(project)
.map(p -> Arrays.asList(p.getProject().getParent()))
.filter(Predicates.notNull())
.orElse(Collections.emptyList());
- try (Repository repository = repositoryManager.openRepository(change.getProject())) {
+ try (Repository repository = repositoryManager.openRepository(project)) {
Set<String> changePaths = new HashSet<>(changeDataFactory.create(change).currentFilePaths());
String branch = change.getDest().branch();
@@ -109,7 +114,9 @@
maybeParentProjectNameKey,
pluginSettings.isBranchDisabled(branch) ? Optional.empty() : Optional.of(branch),
changePaths,
- pluginSettings.expandGroups());
+ pluginSettings.expandGroups(),
+ project.get(),
+ cache);
Map<String, Set<GroupOwner>> fileToOwners =
pluginSettings.expandGroups()
diff --git a/owners/src/main/resources/Documentation/config.md b/owners/src/main/resources/Documentation/config.md
index 62ac73c..e8ae0eb 100644
--- a/owners/src/main/resources/Documentation/config.md
+++ b/owners/src/main/resources/Documentation/config.md
@@ -36,6 +36,26 @@
enableSubmitRequirement = true
```
+cache."owners.path_owners_entries".memoryLimit
+: The cache is used to hold the parsed version of `OWNERS` files in the
+ repository so that when submit rules are calculated (either through prolog
+ or through submit requirements) it is not read over and over again. The
+ cache entry gets invalidated when `OWNERS` file branch is updated.
+ By default it follows default Gerrit's cache memory limit but it makes
+ sense to adjust it as a function of number of project that use the `owners`
+ plugin multiplied by average number of active branches (plus 1 for the
+ refs/meta/config) and average number of directories (as directory hierarchy
+ back to root is checked for the `OWNERS` file existence).
+ _Note that in opposite to the previous settings the modification needs to be
+ performed in the `$GERRIT_SITE/etc/gerrit.config` file._
+
+Example
+
+```
+[cache "owners.path_owners_entries"]
+ memoryLimit = 2048
+```
+
## Configuration
Owner approval is determined based on OWNERS files located in the same
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
index cd23d0d..ec3e816 100644
--- a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
+++ b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
@@ -35,7 +35,7 @@
import org.eclipse.jgit.junit.TestRepository;
import org.junit.Test;
-@TestPlugin(name = "owners", httpModule = "com.googlesource.gerrit.owners.OwnersRestApiModule")
+@TestPlugin(name = "owners", sysModule = "com.googlesource.gerrit.owners.OwnersModule")
@UseLocalDisk
public class GetFilesOwnersIT extends LightweightPluginDaemonTest {
