Post messages about code owner approved files asynchronously by default
When a code owner approves a change the code-owners plugin extends the
change message that Gerrit core posts for the approval to include the
paths that have been code-owner approved by the approval.
Computing the owned path is expensive, so this extension of the change
can be slow which goes at the expense of the post review latency.
To improve the post review latency we add a new configuration option to
post the approved path asynchronously in a separate change message and
turn this configuration on by default.
The new code-owner.enableAsyncMessageOnCodeOwnerApproval configuration
option follows the example of the existing
code-owners.enableAsyncMessageOnAddReviewer configuration which controls
whether the change message with paths owned by a newly added reviewer
should be posted asynchronously.
If asynchronous posting is enabled, 2 changes messages are posted when a
code owner applies an approval:
1. "Patch Set X: Code-Owner+1" posted by Gerrit core
2. "Patch Set X: By voting Code-Review+1 the following files are now
code-owner approved by YYY: ..." posted by the code-owners plugin
If asynchronous posting is disabled, there is only a single change
message:
1. "Patch Set X: Code-Owner+1\n\nBy voting Code-Review+1 the following
files are now code-owner approved by YYY: ..." posted by Gerrit core
and extended by the code-owners plugin
Posting the change message asynchronously in OnCodeOwnerApproval follows
the example of how asynchronous posting is done in
CodeOwnersOnAddReviewer.
For running tests asynchronous message posting is disabled by default.
This is important to avoid LOCK_FAILUREs (see comment in
AbstractCodeOwnersTest.defaultConfig). So far disabling asynchronous
message posting for CodeOwnersOnAddReviewer was only relevant for
integration tests but the OnCodeOwnerApproval logic is also executed by
unit tests, hence we have to pull up this configuration from
AbstractCodeOwnersIT to AbstractCodeOwnersTest and add it to custom
default configs.
The asynchronous message posting is tested by a new test in
OnCodeOwnerApprovalIT which follows the example of the async test in
CodeOwnersOnAddReviewerIT.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I129e611afbb6384f10bd5a42dea43458273ed95d
Bug: Google b/273710181
diff --git a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersIT.java b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersIT.java
index 8ea5a5b..f7a2943 100644
--- a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersIT.java
+++ b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersIT.java
@@ -25,7 +25,6 @@
import com.google.gerrit.plugins.codeowners.api.impl.ProjectCodeOwnersFactory;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackendId;
import com.google.gerrit.plugins.codeowners.backend.config.BackendConfig;
-import com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig;
import com.google.gerrit.plugins.codeowners.backend.proto.ProtoBackend;
import com.google.gerrit.testing.ConfigSuite;
import java.util.Arrays;
@@ -45,26 +44,6 @@
*/
public class AbstractCodeOwnersIT extends AbstractCodeOwnersTest {
/**
- * Returns a {@code gerrit.config} without code owner backend configuration to test the default
- * setup.
- */
- @ConfigSuite.Default
- public static Config defaultConfig() {
- Config cfg = new Config();
-
- // Disable asynchronous posting of change messages during tests to avoid parallel updates to
- // NoteDb and hence risking LOCK_FAILURES (especially needed since the test API does not retry
- // on LOCK_FAILURES).
- cfg.setBoolean(
- "plugin",
- "code-owners",
- GeneralConfig.KEY_ENABLE_ASYNC_MESSAGE_ON_ADD_REVIEWER,
- /* value= */ false);
-
- return cfg;
- }
-
- /**
* Returns a {@code gerrit.config} for every code owner backend so that all code owner backends
* are tested.
*/
diff --git a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java
index fda3e52..1283932 100644
--- a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java
+++ b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java
@@ -43,10 +43,12 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigReference;
import com.google.gerrit.plugins.codeowners.backend.config.BackendConfig;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
+import com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig;
import com.google.gerrit.plugins.codeowners.backend.config.StatusConfig;
import com.google.gerrit.plugins.codeowners.backend.findowners.FindOwnersBackend;
import com.google.gerrit.plugins.codeowners.backend.proto.ProtoBackend;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
+import com.google.gerrit.testing.ConfigSuite;
import com.google.inject.Inject;
import java.nio.file.Path;
import java.util.Map;
@@ -72,6 +74,31 @@
name = "code-owners",
sysModule = "com.google.gerrit.plugins.codeowners.acceptance.TestModule")
public class AbstractCodeOwnersTest extends LightweightPluginDaemonTest {
+ /**
+ * Returns a {@code gerrit.config} without code owner backend configuration to test the default
+ * setup.
+ */
+ @ConfigSuite.Default
+ public static Config defaultConfig() {
+ Config cfg = new Config();
+
+ // Disable asynchronous posting of change messages during tests to avoid parallel updates to
+ // NoteDb and hence risking LOCK_FAILURES (especially needed since the test API does not retry
+ // on LOCK_FAILURES).
+ cfg.setBoolean(
+ "plugin",
+ "code-owners",
+ GeneralConfig.KEY_ENABLE_ASYNC_MESSAGE_ON_ADD_REVIEWER,
+ /* value= */ false);
+ cfg.setBoolean(
+ "plugin",
+ "code-owners",
+ GeneralConfig.KEY_ENABLE_ASYNC_MESSAGE_ON_CODE_OWNER_APPROVAL,
+ /* value= */ false);
+
+ return cfg;
+ }
+
@Inject private ProjectOperations projectOperations;
private CodeOwnerConfigOperations codeOwnerConfigOperations;
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
index 67532c1..e7c03b1 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
@@ -15,6 +15,8 @@
package com.google.gerrit.plugins.codeowners.backend;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.CHANGE_MODIFICATION;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.PLUGIN;
import com.google.common.collect.ImmutableList;
import com.google.common.flogger.FluentLogger;
@@ -25,11 +27,20 @@
import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval;
import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
+import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.git.WorkQueue;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.restapi.change.OnPostReview;
+import com.google.gerrit.server.update.BatchUpdate;
+import com.google.gerrit.server.update.BatchUpdateOp;
+import com.google.gerrit.server.update.ChangeContext;
+import com.google.gerrit.server.update.RetryHelper;
+import com.google.gerrit.server.update.context.RefUpdateContext;
import com.google.gerrit.server.util.AccountTemplateUtil;
import com.google.gerrit.server.util.LabelVote;
+import com.google.gerrit.server.util.ManualRequestContext;
+import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.nio.file.Path;
@@ -55,18 +66,33 @@
class OnCodeOwnerApproval implements OnPostReview {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private static final String TAG_ADD_CODE_OWNER_APPROVAL =
+ ChangeMessagesUtil.AUTOGENERATED_BY_GERRIT_TAG_PREFIX + "code-owners:addCodeOwnerApproval";
+
+ private final WorkQueue workQueue;
+ private final OneOffRequestContext oneOffRequestContext;
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final CodeOwnerApprovalCheck codeOwnerApprovalCheck;
private final CodeOwnerMetrics codeOwnerMetrics;
+ private final ChangeMessagesUtil changeMessageUtil;
+ private final RetryHelper retryHelper;
@Inject
OnCodeOwnerApproval(
+ WorkQueue workQueue,
+ OneOffRequestContext oneOffRequestContext,
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
CodeOwnerApprovalCheck codeOwnerApprovalCheck,
- CodeOwnerMetrics codeOwnerMetrics) {
+ CodeOwnerMetrics codeOwnerMetrics,
+ ChangeMessagesUtil changeMessageUtil,
+ RetryHelper retryHelper) {
+ this.workQueue = workQueue;
+ this.oneOffRequestContext = oneOffRequestContext;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.codeOwnerApprovalCheck = codeOwnerApprovalCheck;
this.codeOwnerMetrics = codeOwnerMetrics;
+ this.changeMessageUtil = changeMessageUtil;
+ this.retryHelper = retryHelper;
}
@Override
@@ -99,6 +125,33 @@
return Optional.empty();
}
+ if (codeOwnersConfig.enableAsyncMessageOnCodeOwnerApproval()) {
+ // post change message asynchronously to avoid adding latency to PostReview
+ logger.atFine().log("schedule asynchronous posting of the change message");
+ @SuppressWarnings("unused")
+ WorkQueue.Task<?> possiblyIgnoredError =
+ (WorkQueue.Task<?>)
+ workQueue
+ .getDefaultQueue()
+ .submit(
+ () -> {
+ try (ManualRequestContext ignored =
+ oneOffRequestContext.openAs(user.getAccountId())) {
+ postChangeMessage(
+ when,
+ user,
+ changeNotes,
+ patchSet,
+ oldApprovals,
+ approvals,
+ requiredApproval,
+ maxPathsInChangeMessage);
+ }
+ });
+ return Optional.empty();
+ }
+
+ logger.atFine().log("post change message synchronously");
try (Timer0.Context ctx = codeOwnerMetrics.extendChangeMessageOnPostReview.start()) {
return buildMessageForCodeOwnerApproval(
user,
@@ -128,6 +181,58 @@
}
}
+ private void postChangeMessage(
+ Instant when,
+ IdentifiedUser user,
+ ChangeNotes changeNotes,
+ PatchSet patchSet,
+ Map<String, Short> oldApprovals,
+ Map<String, Short> approvals,
+ RequiredApproval requiredApproval,
+ int maxPathsInChangeMessages) {
+ try (Timer0.Context ctx = codeOwnerMetrics.addChangeMessageOnCodeOwnerApproval.start()) {
+ retryHelper
+ .changeUpdate(
+ "addCodeOwnersMessageOnCodeOwnerApproval",
+ updateFactory -> {
+ try (BatchUpdate batchUpdate =
+ updateFactory.create(changeNotes.getProjectName(), user, when);
+ RefUpdateContext pluginCtx = RefUpdateContext.open(PLUGIN);
+ RefUpdateContext changeCtx = RefUpdateContext.open(CHANGE_MODIFICATION)) {
+ batchUpdate.addOp(
+ changeNotes.getChangeId(),
+ new Op(
+ user,
+ changeNotes,
+ patchSet,
+ oldApprovals,
+ approvals,
+ requiredApproval,
+ maxPathsInChangeMessages));
+ batchUpdate.execute();
+ }
+ return null;
+ })
+ .call();
+ } catch (Exception e) {
+ Optional<? extends Exception> configurationError =
+ CodeOwnersExceptionHook.getCauseOfConfigurationError(e);
+ if (configurationError.isPresent()) {
+ logger.atWarning().log(
+ "Failed to post code-owners change message for code owner approval on change %s in"
+ + " project %s: %s",
+ changeNotes.getChangeId(),
+ changeNotes.getProjectName(),
+ configurationError.get().getMessage());
+ } else {
+ logger.atSevere().withCause(e).log(
+ "Failed to post code-owners change message for code owner approval on change %s in"
+ + " project %s.",
+ changeNotes.getChangeId(), changeNotes.getProjectName());
+ }
+ }
+ }
+
private Optional<String> buildMessageForCodeOwnerApproval(
IdentifiedUser user,
ChangeNotes changeNotes,
@@ -292,4 +397,48 @@
approvals);
return LabelVote.create(labelName, approvals.get(labelName));
}
+
+ private class Op implements BatchUpdateOp {
+ private final IdentifiedUser user;
+ private final ChangeNotes changeNotes;
+ private final PatchSet patchSet;
+ private final Map<String, Short> oldApprovals;
+ private final Map<String, Short> approvals;
+ private final RequiredApproval requiredApproval;
+ private final int limit;
+
+ Op(
+ IdentifiedUser user,
+ ChangeNotes changeNotes,
+ PatchSet patchSet,
+ Map<String, Short> oldApprovals,
+ Map<String, Short> approvals,
+ RequiredApproval requiredApproval,
+ int limit) {
+ this.user = user;
+ this.changeNotes = changeNotes;
+ this.patchSet = patchSet;
+ this.oldApprovals = oldApprovals;
+ this.approvals = approvals;
+ this.requiredApproval = requiredApproval;
+ this.limit = limit;
+ }
+
+ @Override
+ public boolean updateChange(ChangeContext ctx) throws Exception {
+ Optional<String> message =
+ buildMessageForCodeOwnerApproval(
+ user, changeNotes, patchSet, oldApprovals, approvals, requiredApproval, limit);
+
+ if (message.isEmpty()) {
+ return false;
+ }
+
+ changeMessageUtil.setChangeMessage(
+ ctx,
+ String.format("Patch Set %s: %s", patchSet.id().get(), message.get()),
+ TAG_ADD_CODE_OWNER_APPROVAL);
+ return true;
+ }
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
index f02e54d..786d3e4 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
@@ -84,6 +84,7 @@
@Nullable private FallbackCodeOwners fallbackCodeOwners;
@Nullable private Integer maxPathsInChangeMessages;
@Nullable private Boolean enableAsyncMessageOnAddReviewer;
+ @Nullable private Boolean enableAsyncMessageOnCodeOwnerApproval;
@Nullable private ImmutableSet<CodeOwnerReference> globalCodeOwners;
@Nullable private ImmutableSet<Account.Id> exemptedAccounts;
@Nullable private Optional<String> overrideInfoUrl;
@@ -333,6 +334,18 @@
return enableAsyncMessageOnAddReviewer;
}
+ /**
+ * Gets whether applying a code owner approval should post a change message asynchronously instead
+ * of synchronously extending the change message that is posted by Gerrit core.
+ */
+ public boolean enableAsyncMessageOnCodeOwnerApproval() {
+ if (enableAsyncMessageOnCodeOwnerApproval == null) {
+ enableAsyncMessageOnCodeOwnerApproval =
+ generalConfig.enableAsyncMessageOnCodeOwnerApproval(projectName, pluginConfig);
+ }
+ return enableAsyncMessageOnCodeOwnerApproval;
+ }
+
/** Gets the global code owners. */
public ImmutableSet<CodeOwnerReference> getGlobalCodeOwners() {
if (globalCodeOwners == null) {
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
index 810486f..ada9105 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
@@ -65,6 +65,8 @@
public static final String KEY_FILE_EXTENSION = "fileExtension";
public static final String KEY_ENABLE_ASYNC_MESSAGE_ON_ADD_REVIEWER =
"enableAsyncMessageOnAddReviewer";
+ public static final String KEY_ENABLE_ASYNC_MESSAGE_ON_CODE_OWNER_APPROVAL =
+ "enableAsyncMessageOnCodeOwnerApproval";
public static final String KEY_ENABLE_CODE_OWNER_CONFIG_FILES_WITH_FILE_EXTENSIONS =
"enableCodeOwnerConfigFilesWithFileExtensions";
public static final String KEY_READ_ONLY = "readOnly";
@@ -476,6 +478,25 @@
}
/**
+ * Gets whether applying a code owner approval should post a change message asynchronously instead
+ * of synchronously extending the change message that is posted by Gerrit core.
+ *
+ * @param project the project for which the enable async message on code owner approval
+ * configuration should be read
+ * @param pluginConfig the plugin config from which the enable async message on code owner
+ * approval configuration should be read
+ * @return whether applying a code owner approval should post a change message asynchronously
+ * instead of synchronously extending the change message that is posted by Gerrit core
+ */
+ boolean enableAsyncMessageOnCodeOwnerApproval(Project.NameKey project, Config pluginConfig) {
+ return getBooleanConfig(
+ project,
+ pluginConfig,
+ KEY_ENABLE_ASYNC_MESSAGE_ON_CODE_OWNER_APPROVAL,
+ /* defaultValue= */ true);
+ }
+
+ /**
* Gets the enable validation on branch creation configuration from the given plugin config for
* the specified project with fallback to {@code gerrit.config} and default to {@code true}.
*
diff --git a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
index fa90564..4a6c37d 100644
--- a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
+++ b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
@@ -33,6 +33,7 @@
public class CodeOwnerMetrics {
// latency metrics
public final Timer1<String> addChangeMessageOnAddReviewer;
+ public final Timer0 addChangeMessageOnCodeOwnerApproval;
public final Timer0 computeFileStatus;
public final Timer1<Boolean> computeFileStatuses;
public final Timer0 computeOwnedPaths;
@@ -85,6 +86,11 @@
.description(
"Whether the change message was posted synchronously or asynchronously.")
.build());
+ this.addChangeMessageOnCodeOwnerApproval =
+ createTimer(
+ "add_change_message_on_code_owner_approval",
+ "Latency for asynchronously adding a change message with the owned path when a code"
+ + " owner approval is applied");
this.computeFileStatus =
createTimer("compute_file_status", "Latency for computing the file status of one file");
this.computeFileStatuses =
@@ -105,8 +111,8 @@
this.extendChangeMessageOnPostReview =
createTimer(
"extend_change_message_on_post_review",
- "Latency for extending the change message with the owned path when a code owner"
- + " approval is applied");
+ "Latency for synchronously extending the change message with the owned path when a"
+ + " code owner approval is applied");
this.getChangedFiles =
createTimer("get_changed_files", "Latency for getting changed files from diff cache");
this.prepareFileStatusComputation =
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
index b2a6630..b0caf0a 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
@@ -631,6 +631,27 @@
}
@Test
+ public void configureEnableAsyncMessageOnCodeOwnerApproval() throws Exception {
+ fetchRefsMetaConfig();
+
+ Config cfg = new Config();
+ cfg.setBoolean(
+ CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ GeneralConfig.KEY_ENABLE_ASYNC_MESSAGE_ON_CODE_OWNER_APPROVAL,
+ false);
+ setCodeOwnersConfig(cfg);
+
+ PushResult r = pushRefsMetaConfig();
+ assertThat(r.getRemoteUpdate(RefNames.REFS_CONFIG).getStatus()).isEqualTo(Status.OK);
+ assertThat(
+ codeOwnersPluginConfiguration
+ .getProjectConfig(project)
+ .enableAsyncMessageOnCodeOwnerApproval())
+ .isFalse();
+ }
+
+ @Test
public void cannotSetInvalidMaxPathsInChangeMessages() throws Exception {
fetchRefsMetaConfig();
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
index d3fe19a..7d7a99a 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
@@ -16,7 +16,10 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import com.github.rholder.retry.RetryerBuilder;
+import com.github.rholder.retry.StopStrategies;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
@@ -34,12 +37,22 @@
import com.google.gerrit.server.util.AccountTemplateUtil;
import com.google.gerrit.testing.FakeEmailSender.Message;
import com.google.inject.Inject;
+import java.time.Duration;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
+import java.util.concurrent.Callable;
import org.junit.Test;
-/** Acceptance test for {@code com.google.gerrit.plugins.codeowners.backend.OnCodeOwnerApproval}. */
+/**
+ * Acceptance test for {@code com.google.gerrit.plugins.codeowners.backend.OnCodeOwnerApproval}.
+ *
+ * <p>For tests the change message that is posted when a code owner approval is applied, is added
+ * synchronously by default (see {@link AbstractCodeOwnersIT #defaultConfig()}). Tests that want to
+ * verify the asynchronous posting of this change message need to set {@code
+ * plugin.code-owners.enableAsyncMessageOnCodeOwnerApproval=true} in {@code gerrit.config}
+ * explicitly (by using the {@link GerritConfig} annotation).
+ */
public class OnCodeOwnerApprovalIT extends AbstractCodeOwnersIT {
@Inject private RequestScopeOperations requestScopeOperations;
@Inject private ProjectOperations projectOperations;
@@ -1143,4 +1156,49 @@
+ "* %s\n",
user.fullName(), user.email(), path));
}
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableAsyncMessageOnCodeOwnerApproval", value = "true")
+ public void changeMessageListsNewlyApprovedPaths_async() throws Exception {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ String path = "foo/bar.baz";
+ String changeId = createChange("Test Change", path, "file content").getChangeId();
+
+ recommend(changeId);
+
+ assertAsyncChangeMessage(
+ changeId,
+ String.format(
+ "Patch Set 1: "
+ + "By voting Code-Review+1 the following files are now code-owner approved by"
+ + " %s:\n"
+ + "* %s\n",
+ AccountTemplateUtil.getAccountTemplate(admin.id()), path));
+ }
+
+ private void assertAsyncChangeMessage(String changeId, String expectedChangeMessage)
+ throws Exception {
+ assertAsync(
+ () -> {
+ Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages;
+ assertThat(Iterables.getLast(messages).message).isEqualTo(expectedChangeMessage);
+ return null;
+ });
+ }
+
+ private <T> T assertAsync(Callable<T> assertion) throws Exception {
+ return RetryerBuilder.<T>newBuilder()
+ .retryIfException(t -> true)
+ .withStopStrategy(
+ StopStrategies.stopAfterDelay(Duration.ofSeconds(1).toMillis(), MILLISECONDS))
+ .build()
+ .call(() -> assertion.call());
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
index 5c31eed..b6c3a43 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
@@ -52,7 +52,7 @@
/** Returns a {@code gerrit.config} that configures all users as fallback code owners. */
@ConfigSuite.Default
public static Config defaultConfig() {
- Config cfg = new Config();
+ Config cfg = AbstractCodeOwnersTest.defaultConfig();
cfg.setEnum(
"plugin",
"code-owners",
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java
index a665ad4..43ab903 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java
@@ -46,7 +46,7 @@
/** Returns a {@code gerrit.config} that configures all users as fallback code owners. */
@ConfigSuite.Default
public static Config defaultConfig() {
- Config cfg = new Config();
+ Config cfg = AbstractCodeOwnersTest.defaultConfig();
cfg.setString(
"plugin", "code-owners", OverrideApprovalConfig.KEY_OVERRIDE_APPROVAL, "Owners-Override+1");
return cfg;
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithStickyApprovalsTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithStickyApprovalsTest.java
index fc73235..28f83f5 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithStickyApprovalsTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithStickyApprovalsTest.java
@@ -55,7 +55,7 @@
/** Returns a {@code gerrit.config} that configures all users as fallback code owners. */
@ConfigSuite.Default
public static Config defaultConfig() {
- Config cfg = new Config();
+ Config cfg = AbstractCodeOwnersTest.defaultConfig();
cfg.setBoolean(
"plugin", "code-owners", GeneralConfig.KEY_ENABLE_STICKY_APPROVALS, /* value= */ true);
return cfg;
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
index 970aa74..17663b2 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
@@ -18,6 +18,7 @@
import static com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.DEFAULT_MAX_PATHS_IN_CHANGE_MESSAGES;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_ASYNC_MESSAGE_ON_ADD_REVIEWER;
+import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_ASYNC_MESSAGE_ON_CODE_OWNER_APPROVAL;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_CODE_OWNER_CONFIG_FILES_WITH_FILE_EXTENSIONS;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_IMPLICIT_APPROVALS;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_STICKY_APPROVALS;
@@ -52,12 +53,19 @@
import com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy;
import com.google.gerrit.server.git.validators.CommitValidationMessage;
import com.google.gerrit.server.git.validators.ValidationMessage;
+import com.google.gerrit.testing.ConfigSuite;
import org.eclipse.jgit.lib.Config;
import org.junit.Before;
import org.junit.Test;
/** Tests for {@link com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig}. */
public class GeneralConfigTest extends AbstractCodeOwnersTest {
+ @ConfigSuite.Default
+ public static Config defaultConfig() {
+ // Override the settings from AbstractCodeOwnersTest.defaultConfig().
+ return new Config();
+ }
+
private GeneralConfig generalConfig;
@Before
@@ -2104,4 +2112,76 @@
throws Exception {
assertThat(generalConfig.enableAsyncMessageOnAddReviewer(project, new Config())).isTrue();
}
+
+ @Test
+ public void cannotGetEnableAsyncMessageOnCodeOwnerApprovalForNullProject() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () ->
+ generalConfig.enableAsyncMessageOnCodeOwnerApproval(
+ /* project= */ null, new Config()));
+ assertThat(npe).hasMessageThat().isEqualTo("project");
+ }
+
+ @Test
+ public void cannotGetEnableAsyncMessageOnCodeOwnerApprovalForNullPluginConfig() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () ->
+ generalConfig.enableAsyncMessageOnCodeOwnerApproval(
+ project, /* pluginConfig= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("pluginConfig");
+ }
+
+ @Test
+ public void noEnableAsyncMessageOnCodeOwnerApproval() throws Exception {
+ assertThat(generalConfig.enableAsyncMessageOnCodeOwnerApproval(project, new Config())).isTrue();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableAsyncMessageOnCodeOwnerApproval", value = "false")
+ public void
+ enableAsyncMessageOnCodeOwnerApprovalConfigurationIsRetrievedFromGerritConfigIfNotSpecifiedOnProjectLevel()
+ throws Exception {
+ assertThat(generalConfig.enableAsyncMessageOnCodeOwnerApproval(project, new Config()))
+ .isFalse();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableAsyncMessageOnCodeOwnerApproval", value = "false")
+ public void
+ enableAsyncMessageOnCodeOwnerApprovalConfigurationInPluginConfigOverridesEnableAsyncMessageOnCodeOwnerApprovalConfigurationInGerritConfig()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ KEY_ENABLE_ASYNC_MESSAGE_ON_CODE_OWNER_APPROVAL,
+ "true");
+ assertThat(generalConfig.enableAsyncMessageOnCodeOwnerApproval(project, cfg)).isTrue();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableAsyncMessageOnCodeOwnerApproval", value = "false")
+ public void invalidEnableAsyncMessageOnCodeOwnerApprovalConfigurationInPluginConfigIsIgnored()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ KEY_ENABLE_ASYNC_MESSAGE_ON_CODE_OWNER_APPROVAL,
+ "INVALID");
+ assertThat(generalConfig.enableAsyncMessageOnCodeOwnerApproval(project, cfg)).isFalse();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.enableAsyncMessageOnCodeOwnerApproval",
+ value = "INVALID")
+ public void invalidEnableAsyncMessageOnCodeOwnerApprovalConfigurationInGerritConfigIsIgnored()
+ throws Exception {
+ assertThat(generalConfig.enableAsyncMessageOnCodeOwnerApproval(project, new Config())).isTrue();
+ }
}
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index 80c5b59..63feaf7 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -695,6 +695,22 @@
in `@PLUGIN@.config`.\
By default `true`.
+<a id="pluginCodeOwnersEnableAsyncMessageOnCodeOwnerApproval">plugin.@PLUGIN@.enableAsyncMessageOnCodeOwnerApproval</a>
+: When a code owner approval is applied on a change the @PLUGIN@ plugin
+ posts a change message that lists the paths that are owned by the code
+ owner to inform that these paths are code owner approved now. This
+ setting controls whether this change message should be posted
+ asynchronously instead of synchronously extending the change message
+ that is posted for the approval by Gerrit core.\
+ Posting these change messages asynchronously improves the latency for
+ post review, since it does not need to wait until the owned paths are
+ computed for the code owner (computing the owned path for a user is
+ rather expensive).\
+ Can be overridden per project by setting
+ [codeOwners.enableAsyncMessageOnCodeOwnerApproval](#codeOwnersEnableAsyncMessageOnCodeOwnerApproval)
+ in `@PLUGIN@.config`.\
+ By default `true`.
+
<a id="pluginCodeOwnersMaxCodeOwnerConfigCacheSize">plugin.@PLUGIN@.maxCodeOwnerConfigCacheSize</a>
: When computing code owner file statuses for a change (e.g. to compute
the results for the code owners submit rule) parsed code owner config
@@ -1277,6 +1293,25 @@
[plugin.@PLUGIN@.enableAsyncMessageOnAddReviewer](#pluginCodeOwnersEnableAsyncMessageOnAddReviewer)
in `gerrit.config` is used.
+<a id="codeOwnersEnableAsyncMessageOnCodeOwnerApproval">codeOwners.enableAsyncMessageOnCodeOwnerApproval</a>
+: When a code owner approval is applied on a change the @PLUGIN@ plugin
+ posts a change message that lists the paths that are owned by the code
+ owner to inform that these paths are code owner approved now. This
+ setting controls whether this change message should be posted
+ asynchronously instead of synchronously extending the change message
+ that is posted for the approval by Gerrit core.\
+ Posting these change messages asynchronously improves the latency for
+ post review, since it does not need to wait until the owned paths are
+ computed for the code owner (computing the owned path for a user is
+ rather expensive).\
+ Overrides the global setting
+ [plugin.@PLUGIN@.enableAsyncMessageOnCodeOwnerApproval](#pluginCodeOwnersEnableAsyncMessageOnOwnerApproval)
+ in `gerrit.config` and the `codeOwners.enableAsyncMessageOnCodeOwnerApproval`
+ setting from parent projects.\
+ If not set, the global setting
+ [plugin.@PLUGIN@.enableAsyncMessageOnCodeOwnerApproval](#pluginCodeOwnersEnableAsyncMessageOnCodeOwnerApproval)
+ in `gerrit.config` is used.
+
---
Back to [@PLUGIN@ documentation index](index.html)
diff --git a/resources/Documentation/metrics.md b/resources/Documentation/metrics.md
index b5f7765..5a54abb 100644
--- a/resources/Documentation/metrics.md
+++ b/resources/Documentation/metrics.md
@@ -12,6 +12,9 @@
added as a reviewer.
** `post_type':
Whether the change message was posted synchronously or asynchronously.
+* `add_change_message_on_code_owner_approval`:
+ Latency for asynchronously adding a change message with the owned path when
+ a code owner approval is applied.
* `compute_file_status`:
Latency for computing the file status for one file.
* `compute_file_statuses`:
@@ -25,8 +28,8 @@
* `compute_patch_set_approvals`:
Latency for computing the approvals of the current patch set.
* `extend_change_message_on_post_review`:
- Latency for extending the change message with the owned path when a code owner
- approval is applied.
+ Latency for synchronously extending the change message with the owned path
+ when a code owner approval is applied.
* `get_changed_files`:
Latency for getting changed files from diff cache.
* `prepare_file_status_computation`:
