Load each code owner config only once for computing code owner statuses
When computing code owner statuses we need to iterate over the relevant
code owner configs for each of the files that have been touched in the
change. In this process code owner config files that apply to multiple
files are loaded multiple times (e.g. the root code owner config is
loaded once per file). To reduce the number of code owner config loads
we add a transient in-memory cache that caches the loaded code owner
config files for the time of the code owner computation. Once the
computation is done the cache is dropped.
The implementation of the cache is rather simple. It has a HashMap in
which it stores loaded code owner configs. If a requested code owner
config is available in the map it is returned, otherwise the code owner
config is loaded and put into the cache. The implementation of the cache
is not thread-safe, but since we create a new instance for every request
and the request processes file synchronously it also doesn't need to be
thread-safe.
The cache is instantiated in CodeOwnerConfigHierarchy which loads the
code owner config files that are relevant for a path. Previously
CodeOwnerConfigHierarchy was a singleton, but now the @Singleton
annotation was removed so that each instance gets its own cache. The
cache is passed over into PathCodeOwners which does the actual loading
of code owner configs.
CodeOwnerApprovalCache takes care to instantiate
CodeOwnerConfigHierarchy outside the loop that iterates over all files,
so that the same instance, and hence the same cache, is used for all
files.
Other classes that get CodeOwnerConfigHierarchy injected
(CheckCodeOwner, AbstractGetCodeOwnersForPath) are non-sigleton and
hence don't need to be adapted.
PathCodeOwners also has other callers that do not require caching. Hence
PathCodeOwners can be used, with or without cache. To support this we
add a new CodeOwnerConfigLoader interface that is implemented by
CodeOwners (loads code owner configs from the backend) and the new cache
(retrieves code owner configs from the cache, or falls back to
CodeOwners).
There are no unit tests for the new cache class yet, but the test
coverage shows that it is fully covered by the existing tests (except
the catch block for exception handling).
This change also adds a new metric for counting the number of code owner
configs that have been returned from cache. This way we will be able to
see what difference this change made.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Id72def08ee26b2f7457a4e83e42ee46e7cbec0dc
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index a582589..4e10e8f 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -89,7 +89,7 @@
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final ChangedFiles changedFiles;
private final PureRevertCache pureRevertCache;
- private final CodeOwnerConfigHierarchy codeOwnerConfigHierarchy;
+ private final Provider<CodeOwnerConfigHierarchy> codeOwnerConfigHierarchyProvider;
private final Provider<CodeOwnerResolver> codeOwnerResolver;
private final ApprovalsUtil approvalsUtil;
private final CodeOwnerMetrics codeOwnerMetrics;
@@ -101,7 +101,7 @@
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
ChangedFiles changedFiles,
PureRevertCache pureRevertCache,
- CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
+ Provider<CodeOwnerConfigHierarchy> codeOwnerConfigHierarchyProvider,
Provider<CodeOwnerResolver> codeOwnerResolver,
ApprovalsUtil approvalsUtil,
CodeOwnerMetrics codeOwnerMetrics) {
@@ -110,7 +110,7 @@
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.changedFiles = changedFiles;
this.pureRevertCache = pureRevertCache;
- this.codeOwnerConfigHierarchy = codeOwnerConfigHierarchy;
+ this.codeOwnerConfigHierarchyProvider = codeOwnerConfigHierarchyProvider;
this.codeOwnerResolver = codeOwnerResolver;
this.approvalsUtil = approvalsUtil;
this.codeOwnerMetrics = codeOwnerMetrics;
@@ -285,12 +285,14 @@
FallbackCodeOwners fallbackCodeOwners =
codeOwnersPluginConfiguration.getFallbackCodeOwners(branch.project());
+ CodeOwnerConfigHierarchy codeOwnerConfigHierarchy = codeOwnerConfigHierarchyProvider.get();
return changedFiles
.compute(changeNotes.getProjectName(), changeNotes.getCurrentPatchSet().commitId())
.stream()
.map(
changedFile ->
getFileStatus(
+ codeOwnerConfigHierarchy,
branch,
revision,
globalCodeOwners,
@@ -350,10 +352,12 @@
return getAllPathsAsApproved(changeNotes, patchSet);
}
+ CodeOwnerConfigHierarchy codeOwnerConfigHierarchy = codeOwnerConfigHierarchyProvider.get();
return changedFiles.compute(changeNotes.getProjectName(), patchSet.commitId()).stream()
.map(
changedFile ->
getFileStatus(
+ codeOwnerConfigHierarchy,
branch,
revision,
/* globalCodeOwners= */ CodeOwnerResolverResult.createEmpty(),
@@ -406,6 +410,7 @@
}
private FileCodeOwnerStatus getFileStatus(
+ CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
BranchNameKey branch,
ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
@@ -426,6 +431,7 @@
.map(
newPath ->
getPathCodeOwnerStatus(
+ codeOwnerConfigHierarchy,
branch,
revision,
globalCodeOwners,
@@ -448,6 +454,7 @@
oldPathStatus =
Optional.of(
getPathCodeOwnerStatus(
+ codeOwnerConfigHierarchy,
branch,
revision,
globalCodeOwners,
@@ -468,6 +475,7 @@
}
private PathCodeOwnerStatus getPathCodeOwnerStatus(
+ CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
BranchNameKey branch,
ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
index cecd46d..6b4da27 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
@@ -23,7 +23,6 @@
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.inject.Inject;
-import com.google.inject.Singleton;
import java.io.IOException;
import java.nio.file.Path;
import java.util.Optional;
@@ -44,18 +43,21 @@
* using {@code set noparent} in the root code owner config if the {@code find-owners} backend is
* used).
*/
-@Singleton
public class CodeOwnerConfigHierarchy {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final GitRepositoryManager repoManager;
private final PathCodeOwners.Factory pathCodeOwnersFactory;
+ private final TransientCodeOwnerConfigCache transientCodeOwnerConfigCache;
@Inject
CodeOwnerConfigHierarchy(
- GitRepositoryManager repoManager, PathCodeOwners.Factory pathCodeOwnersFactory) {
+ GitRepositoryManager repoManager,
+ PathCodeOwners.Factory pathCodeOwnersFactory,
+ TransientCodeOwnerConfigCache transientCodeOwnerConfigCache) {
this.repoManager = repoManager;
this.pathCodeOwnersFactory = pathCodeOwnersFactory;
+ this.transientCodeOwnerConfigCache = transientCodeOwnerConfigCache;
}
/**
@@ -153,7 +155,8 @@
CodeOwnerConfig.Key codeOwnerConfigKey =
CodeOwnerConfig.Key.create(branchNameKey, ownerConfigFolder);
Optional<PathCodeOwners> pathCodeOwners =
- pathCodeOwnersFactory.create(codeOwnerConfigKey, revision, absolutePath);
+ pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCache, codeOwnerConfigKey, revision, absolutePath);
if (pathCodeOwners.isPresent()) {
logger.atFine().log("visit code owner config for %s", ownerConfigFolder);
boolean visitFurtherCodeOwnerConfigs = pathCodeOwnersVisitor.visit(pathCodeOwners.get());
@@ -218,7 +221,8 @@
}
RevCommit metaRevision = rw.parseCommit(ref.getObjectId());
Optional<PathCodeOwners> pathCodeOwners =
- pathCodeOwnersFactory.create(metaCodeOwnerConfigKey, metaRevision, absolutePath);
+ pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCache, metaCodeOwnerConfigKey, metaRevision, absolutePath);
if (pathCodeOwners.isPresent()) {
logger.atFine().log("visit code owner config %s", metaCodeOwnerConfigKey);
pathCodeOwnersVisitor.visit(pathCodeOwners.get());
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigLoader.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigLoader.java
new file mode 100644
index 0000000..b263c20
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigLoader.java
@@ -0,0 +1,40 @@
+// Copyright (C) 2021 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.plugins.codeowners.backend;
+
+import java.util.Optional;
+import org.eclipse.jgit.lib.ObjectId;
+
+/** API to load {@link CodeOwnerConfig}s. */
+public interface CodeOwnerConfigLoader {
+ /**
+ * Retrieves the code owner config for the given key from the given branch revision.
+ *
+ * @param codeOwnerConfigKey the key of the code owner config that should be retrieved
+ * @param revision the branch revision from which the code owner config should be loaded
+ * @return the code owner config for the given key if it exists, otherwise {@link
+ * Optional#empty()}
+ */
+ public Optional<CodeOwnerConfig> get(CodeOwnerConfig.Key codeOwnerConfigKey, ObjectId revision);
+
+ /**
+ * Retrieves the code owner config for the given key from the current revision of the branch.
+ *
+ * @param codeOwnerConfigKey the key of the code owner config that should be retrieved
+ * @return the code owner config for the given key if it exists, otherwise {@link
+ * Optional#empty()}
+ */
+ public Optional<CodeOwnerConfig> getFromCurrentRevision(CodeOwnerConfig.Key codeOwnerConfigKey);
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
index a16714c..1fece0d 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
@@ -155,7 +155,8 @@
requireNonNull(codeOwnerConfig, "codeOwnerConfig");
requireNonNull(absolutePath, "absolutePath");
checkState(absolutePath.isAbsolute(), "path %s must be absolute", absolutePath);
- return resolvePathCodeOwners(pathCodeOwnersFactory.create(codeOwnerConfig, absolutePath));
+ return resolvePathCodeOwners(
+ pathCodeOwnersFactory.createWithoutCache(codeOwnerConfig, absolutePath));
}
/**
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwners.java
index e248570..9315523 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwners.java
@@ -36,7 +36,7 @@
* that we avoid code repetition in the code owner backends.
*/
@Singleton
-public class CodeOwners {
+public class CodeOwners implements CodeOwnerConfigLoader {
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final CodeOwnerMetrics codeOwnerMetrics;
@@ -48,14 +48,7 @@
this.codeOwnerMetrics = codeOwnerMetrics;
}
- /**
- * Retrieves the code owner config for the given key from the given branch revision.
- *
- * @param codeOwnerConfigKey the key of the code owner config that should be retrieved
- * @param revision the branch revision from which the code owner config should be loaded
- * @return the code owner config for the given key if it exists, otherwise {@link
- * Optional#empty()}
- */
+ @Override
public Optional<CodeOwnerConfig> get(CodeOwnerConfig.Key codeOwnerConfigKey, ObjectId revision) {
requireNonNull(codeOwnerConfigKey, "codeOwnerConfigKey");
requireNonNull(revision, "revision");
@@ -65,13 +58,7 @@
return codeOwnerBackend.getCodeOwnerConfig(codeOwnerConfigKey, revision);
}
- /**
- * Retrieves the code owner config for the given key from the current revision of the branch.
- *
- * @param codeOwnerConfigKey the key of the code owner config that should be retrieved
- * @return the code owner config for the given key if it exists, otherwise {@link
- * Optional#empty()}
- */
+ @Override
public Optional<CodeOwnerConfig> getFromCurrentRevision(CodeOwnerConfig.Key codeOwnerConfigKey) {
requireNonNull(codeOwnerConfigKey, "codeOwnerConfigKey");
codeOwnerMetrics.countCodeOwnerConfigReads.increment();
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
index 78e315a..c382801 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
@@ -24,6 +24,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Project;
import com.google.gerrit.metrics.Timer0;
@@ -73,11 +74,12 @@
this.codeOwners = codeOwners;
}
- public PathCodeOwners create(CodeOwnerConfig codeOwnerConfig, Path absolutePath) {
+ public PathCodeOwners createWithoutCache(CodeOwnerConfig codeOwnerConfig, Path absolutePath) {
requireNonNull(codeOwnerConfig, "codeOwnerConfig");
return new PathCodeOwners(
codeOwnerMetrics,
projectCache,
+ /* transientCodeOwnerConfigCache= */ null,
codeOwners,
codeOwnerConfig,
absolutePath,
@@ -85,14 +87,21 @@
}
public Optional<PathCodeOwners> create(
- CodeOwnerConfig.Key codeOwnerConfigKey, ObjectId revision, Path absolutePath) {
- return codeOwners
+ TransientCodeOwnerConfigCache transientCodeOwnerConfigCache,
+ CodeOwnerConfig.Key codeOwnerConfigKey,
+ ObjectId revision,
+ Path absolutePath) {
+ requireNonNull(transientCodeOwnerConfigCache, "transientCodeOwnerConfigCache");
+ requireNonNull(codeOwnerConfigKey, "codeOwnerConfigKey");
+ requireNonNull(revision, "revision");
+ return transientCodeOwnerConfigCache
.get(codeOwnerConfigKey, revision)
.map(
codeOwnerConfig ->
new PathCodeOwners(
codeOwnerMetrics,
projectCache,
+ transientCodeOwnerConfigCache,
codeOwners,
codeOwnerConfig,
absolutePath,
@@ -127,6 +136,7 @@
private final CodeOwnerMetrics codeOwnerMetrics;
private final ProjectCache projectCache;
+ private final CodeOwnerConfigLoader codeOwnerConfigLoader;
private final CodeOwners codeOwners;
private final CodeOwnerConfig codeOwnerConfig;
private final Path path;
@@ -137,12 +147,15 @@
private PathCodeOwners(
CodeOwnerMetrics codeOwnerMetrics,
ProjectCache projectCache,
+ @Nullable TransientCodeOwnerConfigCache transientCodeOwnerConfigCache,
CodeOwners codeOwners,
CodeOwnerConfig codeOwnerConfig,
Path path,
PathExpressionMatcher pathExpressionMatcher) {
this.codeOwnerMetrics = requireNonNull(codeOwnerMetrics, "codeOwnerMetrics");
this.projectCache = requireNonNull(projectCache, "projectCache");
+ this.codeOwnerConfigLoader =
+ transientCodeOwnerConfigCache != null ? transientCodeOwnerConfigCache : codeOwners;
this.codeOwners = requireNonNull(codeOwners, "codeOwners");
this.codeOwnerConfig = requireNonNull(codeOwnerConfig, "codeOwnerConfig");
this.path = requireNonNull(path, "path");
@@ -356,8 +369,8 @@
Optional<CodeOwnerConfig> mayBeImportedCodeOwnerConfig =
revision.isPresent()
- ? codeOwners.get(keyOfImportedCodeOwnerConfig, revision.get())
- : codeOwners.getFromCurrentRevision(keyOfImportedCodeOwnerConfig);
+ ? codeOwnerConfigLoader.get(keyOfImportedCodeOwnerConfig, revision.get())
+ : codeOwnerConfigLoader.getFromCurrentRevision(keyOfImportedCodeOwnerConfig);
if (!mayBeImportedCodeOwnerConfig.isPresent()) {
unresolvedImports.add(
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/TransientCodeOwnerConfigCache.java b/java/com/google/gerrit/plugins/codeowners/backend/TransientCodeOwnerConfigCache.java
new file mode 100644
index 0000000..96e0a31
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/TransientCodeOwnerConfigCache.java
@@ -0,0 +1,131 @@
+// Copyright (C) 2021 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.plugins.codeowners.backend;
+
+import com.google.auto.value.AutoValue;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.inject.Inject;
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Optional;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
+
+/**
+ * Class to load and cache {@link CodeOwnerConfig}s within a request.
+ *
+ * <p>This cache is transient, which means the code owner configs stay cached only for the lifetime
+ * of the {@code TransientCodeOwnerConfigCache} instance.
+ *
+ * <p><strong>Note</strong>: This class is not thread-safe.
+ */
+public class TransientCodeOwnerConfigCache implements CodeOwnerConfigLoader {
+ private final GitRepositoryManager repoManager;
+ private final CodeOwners codeOwners;
+ private final CodeOwnerMetrics codeOwnerMetrics;
+ private final HashMap<CacheKey, Optional<CodeOwnerConfig>> cache = new HashMap<>();
+
+ @Inject
+ TransientCodeOwnerConfigCache(
+ GitRepositoryManager repoManager, CodeOwners codeOwners, CodeOwnerMetrics codeOwnerMetrics) {
+ this.repoManager = repoManager;
+ this.codeOwners = codeOwners;
+ this.codeOwnerMetrics = codeOwnerMetrics;
+ }
+
+ /**
+ * Gets the specified code owner config from the cache, if it was previously retrieved. Otherwise
+ * loads and returns the code owner config.
+ */
+ @Override
+ public Optional<CodeOwnerConfig> get(
+ CodeOwnerConfig.Key codeOwnerConfigKey, @Nullable ObjectId revision) {
+ CacheKey cacheKey = CacheKey.create(codeOwnerConfigKey, revision);
+ Optional<CodeOwnerConfig> cachedCodeOwnerConfig = cache.get(cacheKey);
+ if (cachedCodeOwnerConfig != null) {
+ codeOwnerMetrics.countCodeOwnerConfigCacheReads.increment();
+ return cachedCodeOwnerConfig;
+ }
+ return loadAndCache(cacheKey);
+ }
+
+ /**
+ * Gets the specified code owner config from the cache, if it was previously retrieved. Otherwise
+ * loads and returns the code owner config.
+ */
+ @Override
+ public Optional<CodeOwnerConfig> getFromCurrentRevision(CodeOwnerConfig.Key codeOwnerConfigKey) {
+ return get(codeOwnerConfigKey, /* revision= */ null);
+ }
+
+ /** Load a code owner config and puts it into the cache. */
+ private Optional<CodeOwnerConfig> loadAndCache(CacheKey cacheKey) {
+ Optional<CodeOwnerConfig> codeOwnerConfig;
+ if (cacheKey.revision().isPresent()) {
+ codeOwnerConfig = codeOwners.get(cacheKey.codeOwnerConfigKey(), cacheKey.revision().get());
+ } else {
+ Optional<ObjectId> revision = getRevision(cacheKey.codeOwnerConfigKey().branchNameKey());
+ if (revision.isPresent()) {
+ codeOwnerConfig = codeOwners.get(cacheKey.codeOwnerConfigKey(), revision.get());
+ } else {
+ // branch does not exists, hence the code owner config also doesn't exist
+ codeOwnerConfig = Optional.empty();
+ }
+ }
+ cache.put(cacheKey, codeOwnerConfig);
+ return codeOwnerConfig;
+ }
+
+ /**
+ * Gets the revision for the given branch.
+ *
+ * <p>Returns {@link Optional#empty()} if the branch doesn't exist.
+ */
+ private Optional<ObjectId> getRevision(BranchNameKey branchNameKey) {
+ try (Repository repo = repoManager.openRepository(branchNameKey.project())) {
+ Ref ref = repo.exactRef(branchNameKey.branch());
+ if (ref == null) {
+ // branch does not exist
+ return Optional.empty();
+ }
+ return Optional.of(ref.getObjectId());
+ } catch (IOException e) {
+ throw new CodeOwnersInternalServerErrorException(
+ String.format(
+ "failed to get revision of branch %s in project %s",
+ branchNameKey.shortName(), branchNameKey.project()),
+ e);
+ }
+ }
+
+ @AutoValue
+ abstract static class CacheKey {
+ /** The key of the code owner config. */
+ public abstract CodeOwnerConfig.Key codeOwnerConfigKey();
+
+ /** The revision from which the code owner config was loaded. */
+ public abstract Optional<ObjectId> revision();
+
+ public static CacheKey create(
+ CodeOwnerConfig.Key codeOwnerConfigKey, @Nullable ObjectId revision) {
+ return new AutoValue_TransientCodeOwnerConfigCache_CacheKey(
+ codeOwnerConfigKey, Optional.ofNullable(revision));
+ }
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
index 0b7661e..e5a3c58 100644
--- a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
+++ b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
@@ -40,6 +40,7 @@
// counter metrics
public final Counter0 countCodeOwnerConfigReads;
+ public final Counter0 countCodeOwnerConfigCacheReads;
private final MetricMaker metricMaker;
@@ -88,7 +89,13 @@
// counter metrics
this.countCodeOwnerConfigReads =
- createCounter("count_code_owner_config_reads", "Total number of code owner config reads");
+ createCounter(
+ "count_code_owner_config_reads",
+ "Total number of code owner config reads from backend");
+ this.countCodeOwnerConfigCacheReads =
+ createCounter(
+ "count_code_owner_config_cache_reads",
+ "Total number of code owner config reads from cache");
}
private Timer0 createLatencyTimer(String name, String description) {
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
index d01d0fc..d326da6 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
@@ -145,7 +145,9 @@
String.format(
"checking code owner config file %s", codeOwnerConfig.key().format(codeOwners)));
OptionalResultWithMessages<PathCodeOwnersResult> pathCodeOwnersResult =
- pathCodeOwnersFactory.create(codeOwnerConfig, absolutePath).resolveCodeOwnerConfig();
+ pathCodeOwnersFactory
+ .createWithoutCache(codeOwnerConfig, absolutePath)
+ .resolveCodeOwnerConfig();
messages.addAll(pathCodeOwnersResult.messages());
pathCodeOwnersResult
.get()
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
index 5490e2e..8929a72 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
@@ -41,6 +41,7 @@
import com.google.gerrit.server.IdentifiedUser;
import com.google.inject.Inject;
import com.google.inject.Key;
+import com.google.inject.Provider;
import com.google.inject.util.Providers;
import java.nio.file.Path;
import java.nio.file.Paths;
@@ -58,6 +59,7 @@
private CodeOwnerConfigOperations codeOwnerConfigOperations;
private PathCodeOwners.Factory pathCodeOwnersFactory;
private DynamicMap<CodeOwnerBackend> codeOwnerBackends;
+ private Provider<TransientCodeOwnerConfigCache> transientCodeOwnerConfigCacheProvider;
@Before
public void setUpCodeOwnersPlugin() throws Exception {
@@ -66,12 +68,14 @@
pathCodeOwnersFactory = plugin.getSysInjector().getInstance(PathCodeOwners.Factory.class);
codeOwnerBackends =
plugin.getSysInjector().getInstance(new Key<DynamicMap<CodeOwnerBackend>>() {});
+ transientCodeOwnerConfigCacheProvider =
+ plugin.getSysInjector().getInstance(new Key<Provider<TransientCodeOwnerConfigCache>>() {});
}
@Test
public void createPathCodeOwnersForCodeOwnerConfig() throws Exception {
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(
+ pathCodeOwnersFactory.createWithoutCache(
createCodeOwnerBuilder().build(), Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners).isNotNull();
}
@@ -82,7 +86,7 @@
assertThrows(
NullPointerException.class,
() ->
- pathCodeOwnersFactory.create(
+ pathCodeOwnersFactory.createWithoutCache(
/* codeOwnerConfig= */ null, Paths.get("/foo/bar/baz.md")));
assertThat(npe).hasMessageThat().isEqualTo("codeOwnerConfig");
}
@@ -93,7 +97,9 @@
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> pathCodeOwnersFactory.create(codeOwnerConfig, /* absolutePath= */ null));
+ () ->
+ pathCodeOwnersFactory.createWithoutCache(
+ codeOwnerConfig, /* absolutePath= */ null));
assertThat(npe).hasMessageThat().isEqualTo("path");
}
@@ -104,7 +110,8 @@
IllegalStateException exception =
assertThrows(
IllegalStateException.class,
- () -> pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get(relativePath)));
+ () ->
+ pathCodeOwnersFactory.createWithoutCache(codeOwnerConfig, Paths.get(relativePath)));
assertThat(exception)
.hasMessageThat()
.isEqualTo(String.format("path %s must be absolute", relativePath));
@@ -123,6 +130,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
codeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md"));
@@ -130,12 +138,28 @@
}
@Test
+ public void cannotCreatePathCodeOwnersForNullCache() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () ->
+ pathCodeOwnersFactory.create(
+ /* transientCodeOwnerConfigCache= */ null,
+ CodeOwnerConfig.Key.create(
+ BranchNameKey.create(project, "master"), Paths.get("/")),
+ projectOperations.project(project).getHead("master"),
+ Paths.get("/foo/bar/baz.md")));
+ assertThat(npe).hasMessageThat().isEqualTo("transientCodeOwnerConfigCache");
+ }
+
+ @Test
public void cannotCreatePathCodeOwnersForNullCodeOwnerConfigKey() throws Exception {
NullPointerException npe =
assertThrows(
NullPointerException.class,
() ->
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
/* codeOwnerConfigKey= */ null,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md")));
@@ -149,6 +173,7 @@
NullPointerException.class,
() ->
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
CodeOwnerConfig.Key.create(
BranchNameKey.create(project, "master"), Paths.get("/")),
/* revision= */ null,
@@ -172,6 +197,7 @@
NullPointerException.class,
() ->
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
codeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
/* absolutePath= */ null));
@@ -195,6 +221,7 @@
IllegalStateException.class,
() ->
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
codeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get(relativePath)));
@@ -207,7 +234,8 @@
public void getEmptyPathCodeOwners() throws Exception {
CodeOwnerConfig emptyCodeOwnerConfig = createCodeOwnerBuilder().build();
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(emptyCodeOwnerConfig, Paths.get("/foo/bar/baz.md"));
+ pathCodeOwnersFactory.createWithoutCache(
+ emptyCodeOwnerConfig, Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().getPathCodeOwners()).isEmpty();
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().hasUnresolvedImports()).isFalse();
}
@@ -219,7 +247,7 @@
.addCodeOwnerSet(CodeOwnerSet.createWithoutPathExpressions(admin.email(), user.email()))
.build();
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
+ pathCodeOwnersFactory.createWithoutCache(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().getPathCodeOwners())
.comparingElementsUsing(hasEmail())
.containsExactly(admin.email(), user.email());
@@ -257,7 +285,7 @@
.addCodeOwnerSet(nonMatchingCodeOwnerSet)
.build();
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
+ pathCodeOwnersFactory.createWithoutCache(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().getPathCodeOwners())
.comparingElementsUsing(hasEmail())
.containsExactly(admin.email(), user.email());
@@ -278,7 +306,7 @@
.build())
.build();
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
+ pathCodeOwnersFactory.createWithoutCache(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().getPathCodeOwners()).isEmpty();
}
}
@@ -310,7 +338,7 @@
.addCodeOwnerSet(globalCodeOwnerSet)
.build();
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
+ pathCodeOwnersFactory.createWithoutCache(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().getPathCodeOwners())
.comparingElementsUsing(hasEmail())
.containsExactly(admin.email());
@@ -344,7 +372,7 @@
.addCodeOwnerSet(globalCodeOwnerSet)
.build();
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
+ pathCodeOwnersFactory.createWithoutCache(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().getPathCodeOwners())
.comparingElementsUsing(hasEmail())
.containsExactly(admin.email(), user.email());
@@ -379,7 +407,7 @@
.addCodeOwnerSet(perFileCodeOwnerSet2)
.build();
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
+ pathCodeOwnersFactory.createWithoutCache(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().getPathCodeOwners())
.comparingElementsUsing(hasEmail())
.containsExactly(admin.email(), user.email());
@@ -390,7 +418,7 @@
public void checkThatParentCodeOwnersAreIgnoredIfCodeOwnerConfigIgnoresParentCodeOwners()
throws Exception {
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(
+ pathCodeOwnersFactory.createWithoutCache(
createCodeOwnerBuilder().setIgnoreParentCodeOwners().build(),
Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().ignoreParentCodeOwners()).isTrue();
@@ -400,7 +428,7 @@
public void checkThatParentCodeOwnersAreNotIgnoredIfCodeOwnerConfigDoesNotIgnoreParentCodeOwners()
throws Exception {
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(
+ pathCodeOwnersFactory.createWithoutCache(
createCodeOwnerBuilder().setIgnoreParentCodeOwners(false).build(),
Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().ignoreParentCodeOwners()).isFalse();
@@ -410,7 +438,7 @@
public void checkThatParentCodeOwnersAreIgnoredIfMatchingCodeOwnerSetIgnoresParentCodeOwners()
throws Exception {
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(
+ pathCodeOwnersFactory.createWithoutCache(
createCodeOwnerBuilder()
.addCodeOwnerSet(
CodeOwnerSet.builder()
@@ -427,7 +455,7 @@
checkThatParentCodeOwnersAreNotIgnoredIfNonMatchingCodeOwnerSetIgnoresParentCodeOwners()
throws Exception {
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(
+ pathCodeOwnersFactory.createWithoutCache(
createCodeOwnerBuilder()
.addCodeOwnerSet(
CodeOwnerSet.builder()
@@ -456,6 +484,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
importingCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -502,6 +531,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
importingCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -549,6 +579,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -596,6 +627,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -645,6 +677,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -693,6 +726,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -745,6 +779,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -796,6 +831,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -839,6 +875,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -877,6 +914,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -936,6 +974,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -995,6 +1034,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -1048,6 +1088,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
importingCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -1086,6 +1127,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -1134,7 +1176,11 @@
.update();
Optional<PathCodeOwners> pathCodeOwners =
- pathCodeOwnersFactory.create(rootCodeOwnerConfigKey, oldRevision, Paths.get("/foo.md"));
+ pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
+ rootCodeOwnerConfigKey,
+ oldRevision,
+ Paths.get("/foo.md"));
assertThat(pathCodeOwners).isPresent();
// Expectation: we get the global owners from the importing and the imported code owner config
@@ -1169,6 +1215,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md"));
@@ -1200,6 +1247,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md"));
@@ -1243,6 +1291,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md"));
@@ -1276,6 +1325,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md"));
@@ -1317,6 +1367,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md"));
@@ -1366,6 +1417,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead(branchName),
Paths.get("/foo.md"));
@@ -1411,6 +1463,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md"));
@@ -1458,6 +1511,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md"));
@@ -1493,6 +1547,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
importingCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -1545,6 +1600,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
importingCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -1608,6 +1664,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -1668,6 +1725,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -1743,6 +1801,7 @@
// *.txt
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.xyz"));
@@ -1752,6 +1811,7 @@
// Expectation for foo.md file: code owners contains only user since foo.md only matches *.md
pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -1764,6 +1824,7 @@
// *.txt
pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.txt"));
diff --git a/resources/Documentation/metrics.md b/resources/Documentation/metrics.md
index eda1fbe..d236b3e 100644
--- a/resources/Documentation/metrics.md
+++ b/resources/Documentation/metrics.md
@@ -31,7 +31,9 @@
## <a id="counterMetrics"> Counter Metrics
* `count_code_owner_config_reads`:
- Total number of code owner config reads.
+ Total number of code owner config reads from backend.
+* `count_code_owner_config_cache_reads`:
+ Total number of code owner config reads from cache.
---
