Reduce number of CodeOwnersPluginConfiguration#getProjectConfig calls
Each call of CodeOwnersPluginConfiguration#getProjectConfig causes a
load of the plugin config with inheritance via
pluginConfigFactory#getProjectPluginConfigWithInheritance(projectName,
pluginName) which is not free. CodeOwnersPluginConfigSnapshot that is
returned by CodeOwnersPluginConfiguration#getProjectConfig caches the
plugin config, hence creating CodeOwnersPluginConfigSnapshot only once
and reading multiple config param from it is more efficient. Reduce the
number of CodeOwnersPluginConfiguration#getProjectConfig calls by
calling it only once in methods that need to read multiple config
parameters. Further improvements will be done in follow-up changes.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I5a34a35f34697280794631c261290e73059996ff
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 453ef46..c25fe52 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -34,6 +34,7 @@
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.metrics.Timer0;
+import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfigSnapshot;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval;
import com.google.gerrit.plugins.codeowners.common.ChangedFile;
@@ -234,17 +235,14 @@
"prepare stream to compute file statuses (project = %s, change = %d)",
changeNotes.getProjectName(), changeNotes.getChangeId().get());
- if (codeOwnersPluginConfiguration
- .getProjectConfig(changeNotes.getProjectName())
- .arePureRevertsExempted()
- && isPureRevert(changeNotes)) {
+ CodeOwnersPluginConfigSnapshot codeOwnersConfig =
+ codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName());
+
+ if (codeOwnersConfig.arePureRevertsExempted() && isPureRevert(changeNotes)) {
return getAllPathsAsApproved(changeNotes, changeNotes.getCurrentPatchSet());
}
- boolean enableImplicitApprovalFromUploader =
- codeOwnersPluginConfiguration
- .getProjectConfig(changeNotes.getProjectName())
- .areImplicitApprovalsEnabled();
+ boolean enableImplicitApprovalFromUploader = codeOwnersConfig.areImplicitApprovalsEnabled();
Account.Id patchSetUploader = changeNotes.getCurrentPatchSet().uploader();
logger.atFine().log(
"patchSetUploader = %d, implicit approval from uploader is %s",
@@ -253,16 +251,10 @@
ImmutableList<PatchSetApproval> currentPatchSetApprovals =
getCurrentPatchSetApprovals(changeNotes);
- RequiredApproval requiredApproval =
- codeOwnersPluginConfiguration
- .getProjectConfig(changeNotes.getProjectName())
- .getRequiredApproval();
+ RequiredApproval requiredApproval = codeOwnersConfig.getRequiredApproval();
logger.atFine().log("requiredApproval = %s", requiredApproval);
- ImmutableSet<RequiredApproval> overrideApprovals =
- codeOwnersPluginConfiguration
- .getProjectConfig(changeNotes.getProjectName())
- .getOverrideApproval();
+ ImmutableSet<RequiredApproval> overrideApprovals = codeOwnersConfig.getOverrideApproval();
boolean hasOverride =
hasOverride(currentPatchSetApprovals, overrideApprovals, changeNotes, patchSetUploader);
logger.atFine().log(
@@ -294,8 +286,7 @@
currentPatchSetApprovals, requiredApproval, changeNotes, patchSetUploader);
logger.atFine().log("reviewers = %s, approvers = %s", reviewerAccountIds, approverAccountIds);
- FallbackCodeOwners fallbackCodeOwners =
- codeOwnersPluginConfiguration.getProjectConfig(branch.project()).getFallbackCodeOwners();
+ FallbackCodeOwners fallbackCodeOwners = codeOwnersConfig.getFallbackCodeOwners();
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy = codeOwnerConfigHierarchyProvider.get();
return changedFiles
@@ -347,10 +338,10 @@
changeNotes.getChangeId().get(),
patchSet.id().get());
- RequiredApproval requiredApproval =
- codeOwnersPluginConfiguration
- .getProjectConfig(changeNotes.getProjectName())
- .getRequiredApproval();
+ CodeOwnersPluginConfigSnapshot codeOwnersConfig =
+ codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName());
+
+ RequiredApproval requiredApproval = codeOwnersConfig.getRequiredApproval();
logger.atFine().log("requiredApproval = %s", requiredApproval);
BranchNameKey branch = changeNotes.getChange().getDest();
@@ -358,8 +349,7 @@
logger.atFine().log("dest branch %s has revision %s", branch.branch(), revision.name());
boolean isProjectOwner = isProjectOwner(changeNotes.getProjectName(), accountId);
- FallbackCodeOwners fallbackCodeOwners =
- codeOwnersPluginConfiguration.getProjectConfig(branch.project()).getFallbackCodeOwners();
+ FallbackCodeOwners fallbackCodeOwners = codeOwnersConfig.getFallbackCodeOwners();
logger.atFine().log(
"fallbackCodeOwner = %s, isProjectOwner = %s", fallbackCodeOwners, isProjectOwner);
if (fallbackCodeOwners.equals(FallbackCodeOwners.PROJECT_OWNERS) && isProjectOwner) {
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
index 294fd69..a7c2f27 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
@@ -25,6 +25,7 @@
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.events.ReviewerAddedListener;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfigSnapshot;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.gerrit.server.ChangeMessagesUtil;
@@ -88,11 +89,10 @@
Change.Id changeId = Change.id(event.getChange()._number);
Project.NameKey projectName = Project.nameKey(event.getChange().project);
- if (codeOwnersPluginConfiguration
- .getProjectConfig(projectName)
- .isDisabled(event.getChange().branch)
- || codeOwnersPluginConfiguration.getProjectConfig(projectName).getMaxPathsInChangeMessages()
- <= 0) {
+ CodeOwnersPluginConfigSnapshot codeOwnersConfig =
+ codeOwnersPluginConfiguration.getProjectConfig(projectName);
+ if (codeOwnersConfig.isDisabled(event.getChange().branch)
+ || codeOwnersConfig.getMaxPathsInChangeMessages() <= 0) {
return;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
index 5cbde72..7f2cd2b 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
@@ -20,6 +20,7 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfigSnapshot;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
@@ -69,9 +70,9 @@
PatchSet patchSet,
Map<String, Short> oldApprovals,
Map<String, Short> approvals) {
- if (codeOwnersPluginConfiguration
- .getProjectConfig(changeNotes.getProjectName())
- .isDisabled(changeNotes.getChange().getDest().branch())) {
+ CodeOwnersPluginConfigSnapshot codeOwnersConfig =
+ codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName());
+ if (codeOwnersConfig.isDisabled(changeNotes.getChange().getDest().branch())) {
return Optional.empty();
}
@@ -80,10 +81,7 @@
return Optional.empty();
}
- RequiredApproval requiredApproval =
- codeOwnersPluginConfiguration
- .getProjectConfig(changeNotes.getProjectName())
- .getRequiredApproval();
+ RequiredApproval requiredApproval = codeOwnersConfig.getRequiredApproval();
if (oldApprovals.get(requiredApproval.labelType().getName()) == null) {
// If oldApprovals doesn't contain the label or if the labels value in it is null, the label
@@ -103,10 +101,9 @@
Map<String, Short> oldApprovals,
Map<String, Short> approvals,
RequiredApproval requiredApproval) {
- int maxPathsInChangeMessage =
- codeOwnersPluginConfiguration
- .getProjectConfig(changeNotes.getProjectName())
- .getMaxPathsInChangeMessages();
+ CodeOwnersPluginConfigSnapshot codeOwnersConfig =
+ codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName());
+ int maxPathsInChangeMessage = codeOwnersConfig.getMaxPathsInChangeMessages();
if (maxPathsInChangeMessage <= 0) {
return Optional.empty();
}
@@ -143,9 +140,7 @@
}
boolean hasImplicitApprovalByUser =
- codeOwnersPluginConfiguration
- .getProjectConfig(changeNotes.getProjectName())
- .areImplicitApprovalsEnabled()
+ codeOwnersConfig.areImplicitApprovalsEnabled()
&& patchSet.uploader().equals(user.getAccountId());
boolean noLongerExplicitlyApproved = false;
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java
index bf15ef2..c06e760 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java
@@ -21,6 +21,7 @@
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfigSnapshot;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval;
import com.google.gerrit.server.IdentifiedUser;
@@ -57,9 +58,9 @@
PatchSet patchSet,
Map<String, Short> oldApprovals,
Map<String, Short> approvals) {
- if (codeOwnersPluginConfiguration
- .getProjectConfig(changeNotes.getProjectName())
- .isDisabled(changeNotes.getChange().getDest().branch())) {
+ CodeOwnersPluginConfigSnapshot codeOwnersConfig =
+ codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName());
+ if (codeOwnersConfig.isDisabled(changeNotes.getChange().getDest().branch())) {
return Optional.empty();
}
@@ -69,8 +70,7 @@
}
ImmutableList<RequiredApproval> overrideApprovals =
- codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName())
- .getOverrideApproval().stream()
+ codeOwnersConfig.getOverrideApproval().stream()
.sorted(comparing(RequiredApproval::toString))
.collect(toImmutableList());
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java
index 2f5856a..e233344 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java
@@ -33,6 +33,7 @@
import com.google.gerrit.plugins.codeowners.api.GeneralInfo;
import com.google.gerrit.plugins.codeowners.api.RequiredApprovalInfo;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackendId;
+import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfigSnapshot;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -78,12 +79,12 @@
}
CodeOwnerBranchConfigInfo format(BranchResource branchResource) {
+ CodeOwnersPluginConfigSnapshot codeOwnersConfig =
+ codeOwnersPluginConfiguration.getProjectConfig(branchResource.getNameKey());
+
CodeOwnerBranchConfigInfo info = new CodeOwnerBranchConfigInfo();
- boolean disabled =
- codeOwnersPluginConfiguration
- .getProjectConfig(branchResource.getNameKey())
- .isDisabled(branchResource.getBranchKey().branch());
+ boolean disabled = codeOwnersConfig.isDisabled(branchResource.getBranchKey().branch());
info.disabled = disabled ? disabled : null;
if (disabled) {
@@ -93,10 +94,7 @@
info.general = formatGeneralInfo(branchResource.getNameKey());
info.backendId =
CodeOwnerBackendId.getBackendId(
- codeOwnersPluginConfiguration
- .getProjectConfig(branchResource.getNameKey())
- .getBackend(branchResource.getBranchKey().branch())
- .getClass());
+ codeOwnersConfig.getBackend(branchResource.getBranchKey().branch()).getClass());
info.requiredApproval = formatRequiredApprovalInfo(branchResource.getNameKey());
info.overrideApproval = formatOverrideApprovalInfo(branchResource.getNameKey());
@@ -104,22 +102,15 @@
}
private GeneralInfo formatGeneralInfo(Project.NameKey projectName) {
+ CodeOwnersPluginConfigSnapshot codeOwnersConfig =
+ codeOwnersPluginConfiguration.getProjectConfig(projectName);
+
GeneralInfo generalInfo = new GeneralInfo();
- generalInfo.fileExtension =
- codeOwnersPluginConfiguration.getProjectConfig(projectName).getFileExtension().orElse(null);
- generalInfo.mergeCommitStrategy =
- codeOwnersPluginConfiguration.getProjectConfig(projectName).getMergeCommitStrategy();
- generalInfo.implicitApprovals =
- codeOwnersPluginConfiguration.getProjectConfig(projectName).areImplicitApprovalsEnabled()
- ? true
- : null;
- generalInfo.overrideInfoUrl =
- codeOwnersPluginConfiguration
- .getProjectConfig(projectName)
- .getOverrideInfoUrl()
- .orElse(null);
- generalInfo.fallbackCodeOwners =
- codeOwnersPluginConfiguration.getProjectConfig(projectName).getFallbackCodeOwners();
+ generalInfo.fileExtension = codeOwnersConfig.getFileExtension().orElse(null);
+ generalInfo.mergeCommitStrategy = codeOwnersConfig.getMergeCommitStrategy();
+ generalInfo.implicitApprovals = codeOwnersConfig.areImplicitApprovalsEnabled() ? true : null;
+ generalInfo.overrideInfoUrl = codeOwnersConfig.getOverrideInfoUrl().orElse(null);
+ generalInfo.fallbackCodeOwners = codeOwnersConfig.getFallbackCodeOwners();
return generalInfo;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index 8af3da0..3f79135 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -38,6 +38,7 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException;
import com.google.gerrit.plugins.codeowners.backend.PathCodeOwners;
+import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfigSnapshot;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.InvalidPluginConfigurationException;
import com.google.gerrit.plugins.codeowners.common.ChangedFile;
@@ -305,9 +306,9 @@
RevWalk revWalk,
RevCommit revCommit,
IdentifiedUser user) {
- if (codeOwnersPluginConfiguration
- .getProjectConfig(branchNameKey.project())
- .isDisabled(branchNameKey.branch())) {
+ CodeOwnersPluginConfigSnapshot codeOwnersConfig =
+ codeOwnersPluginConfiguration.getProjectConfig(branchNameKey.project());
+ if (codeOwnersConfig.isDisabled(branchNameKey.branch())) {
return Optional.of(
ValidationResult.create(
pluginName,
@@ -315,9 +316,7 @@
new CommitValidationMessage(
"code-owners functionality is disabled", ValidationMessage.Type.HINT)));
}
- if (codeOwnersPluginConfiguration
- .getProjectConfig(branchNameKey.project())
- .areCodeOwnerConfigsReadOnly()) {
+ if (codeOwnersConfig.areCodeOwnerConfigsReadOnly()) {
return Optional.of(
ValidationResult.create(
pluginName,
@@ -328,10 +327,7 @@
}
try {
- CodeOwnerBackend codeOwnerBackend =
- codeOwnersPluginConfiguration
- .getProjectConfig(branchNameKey.project())
- .getBackend(branchNameKey.branch());
+ CodeOwnerBackend codeOwnerBackend = codeOwnersConfig.getBackend(branchNameKey.branch());
// For merge commits, always do the comparison against the destination branch
// (MergeCommitStrategy.ALL_CHANGED_FILES). Doing the comparison against the auto-merge
