Allow async assignment of reviewers
In heavy-loaded system the addition of reviewers
in-sync with the NoteDb update may lead to random
LOCK failures on the meta-ref.
Allow to configure the async execution using a new
$GERRIT_SITE/etc/owners-autoassign.config file that
enables:
- async execution
- initial delay
- retry count
- retry delay
Change-Id: I7269e857e5f40f23a91fe6d9ac1b28b467ab7fd5
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AsyncReviewerManager.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AsyncReviewerManager.java
new file mode 100644
index 0000000..6f8f161
--- /dev/null
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AsyncReviewerManager.java
@@ -0,0 +1,103 @@
+// 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.googlesource.gerrit.owners.common;
+
+import com.google.gerrit.extensions.api.changes.ChangeApi;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Account.Id;
+import com.google.gerrit.server.git.WorkQueue;
+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.util.Collection;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Singleton
+class AsyncReviewerManager implements ReviewerManager {
+
+ private static final Logger log = LoggerFactory.getLogger(AsyncReviewerManager.class);
+
+ private final SyncReviewerManager syncReviewerManager;
+ private final AutoAssignConfig config;
+ private final ScheduledExecutorService executor;
+ private final OneOffRequestContext requestContext;
+
+ class AddReviewersTask implements Runnable {
+ private final ChangeApi cApi;
+ private final Collection<Id> reviewers;
+ private final int changeNum;
+ private int retryNum;
+
+ public AddReviewersTask(ChangeApi cApi, Collection<Account.Id> reviewers)
+ throws RestApiException {
+ this.cApi = cApi;
+ this.changeNum = cApi.get()._number;
+ this.reviewers = reviewers;
+ }
+
+ @Override
+ public String toString() {
+ return "auto-assign reviewers to change "
+ + changeNum
+ + (retryNum > 0 ? "(retry #" + retryNum + ")" : "");
+ }
+
+ @Override
+ public void run() {
+ try (ManualRequestContext ctx = requestContext.open()) {
+ syncReviewerManager.addReviewers(cApi, reviewers);
+ } catch (Exception e) {
+ retryNum++;
+
+ if (retryNum > config.retryCount()) {
+ log.error("{} *FAILED*", this, e);
+ } else {
+ long retryInterval = config.retryInterval();
+ log.warn("{} *FAILED*: retrying after {} msec", this, retryInterval, e);
+ executor.schedule(this, retryInterval, TimeUnit.MILLISECONDS);
+ }
+ }
+ }
+ }
+
+ @Inject
+ public AsyncReviewerManager(
+ AutoAssignConfig config,
+ WorkQueue executorFactory,
+ SyncReviewerManager syncReviewerManager,
+ OneOffRequestContext requestContext) {
+ this.config = config;
+ this.syncReviewerManager = syncReviewerManager;
+ this.executor = executorFactory.createQueue(config.asyncThreads(), "AsyncReviewerManager");
+ this.requestContext = requestContext;
+ }
+
+ @Override
+ public void addReviewers(ChangeApi cApi, Collection<Account.Id> reviewers)
+ throws ReviewerManagerException {
+ try {
+ executor.schedule(
+ new AddReviewersTask(cApi, reviewers), config.asyncDelay(), TimeUnit.MILLISECONDS);
+ } catch (RestApiException e) {
+ throw new ReviewerManagerException(e);
+ }
+ }
+}
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AutoAssignConfig.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AutoAssignConfig.java
new file mode 100644
index 0000000..32bdedc
--- /dev/null
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AutoAssignConfig.java
@@ -0,0 +1,81 @@
+// 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.googlesource.gerrit.owners.common;
+
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+
+import com.google.gerrit.extensions.annotations.PluginName;
+import com.google.gerrit.server.config.ConfigUtil;
+import com.google.gerrit.server.config.PluginConfigFactory;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import org.eclipse.jgit.lib.Config;
+
+@Singleton
+public class AutoAssignConfig {
+ public static final String REVIEWERS_SECTION = "reviewers";
+ public static final String ASYNC = "async";
+ public static final boolean ASYNC_DEF = false;
+ public static final String DELAY = "delay";
+ public static final long DELAY_MSEC_DEF = 1500L;
+ public static final String RETRY_COUNT = "retryCount";
+ public static final int RETRY_COUNT_DEF = 2;
+ public static final String RETRY_INTERVAL = "retryInterval";
+ public static final long RETRY_INTERVAL_MSEC_DEF = DELAY_MSEC_DEF;
+ public static final String THREADS = "threads";
+ public static final int THREADS_DEF = 1;
+
+ private final boolean asyncReviewers;
+ private final int asyncThreads;
+ private final int retryCount;
+ private final long retryInterval;
+ private final long asyncDelay;
+
+ @Inject
+ AutoAssignConfig(PluginConfigFactory configFactory, @PluginName String pluginName) {
+ Config config = configFactory.getGlobalPluginConfig(pluginName);
+ asyncReviewers = config.getBoolean(REVIEWERS_SECTION, ASYNC, ASYNC_DEF);
+ asyncThreads = config.getInt(REVIEWERS_SECTION, THREADS, THREADS_DEF);
+ asyncDelay =
+ ConfigUtil.getTimeUnit(
+ config, REVIEWERS_SECTION, null, DELAY, DELAY_MSEC_DEF, MILLISECONDS);
+
+ retryCount = config.getInt(REVIEWERS_SECTION, RETRY_COUNT, RETRY_COUNT_DEF);
+ retryInterval =
+ ConfigUtil.getTimeUnit(
+ config, REVIEWERS_SECTION, null, RETRY_INTERVAL, asyncDelay, MILLISECONDS);
+ }
+
+ public boolean isAsyncReviewers() {
+ return asyncReviewers;
+ }
+
+ public int asyncThreads() {
+ return asyncThreads;
+ }
+
+ public int retryCount() {
+ return retryCount;
+ }
+
+ public long retryInterval() {
+ return retryInterval;
+ }
+
+ public long asyncDelay() {
+ return asyncDelay;
+ }
+}
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 d6bdaef..0278c88 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
@@ -19,10 +19,20 @@
import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
public class AutoassignModule extends AbstractModule {
+ private final AutoAssignConfig config;
+
+ @Inject
+ AutoassignModule(AutoAssignConfig config) {
+ this.config = config;
+ }
+
@Override
protected void configure() {
+ 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/ReviewerManager.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java
index 3335144..2ae2fcd 100644
--- a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java
@@ -1,5 +1,4 @@
-// Copyright (c) 2013 VMware, Inc. All Rights Reserved.
-// Copyright (C) 2017 The Android Open Source Project
+// 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.
@@ -16,92 +15,12 @@
package com.googlesource.gerrit.owners.common;
-import com.google.gerrit.extensions.api.GerritApi;
-import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.api.changes.ChangeApi;
-import com.google.gerrit.extensions.api.changes.ReviewInput;
-import com.google.gerrit.extensions.common.ChangeInfo;
-import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.Account.Id;
-import com.google.gerrit.reviewdb.client.Change;
-import com.google.gerrit.reviewdb.client.Project;
-import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.permissions.ChangePermission;
-import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.util.ManualRequestContext;
-import com.google.gerrit.server.util.OneOffRequestContext;
-import com.google.gwtorm.server.OrmException;
-import com.google.inject.Inject;
-import com.google.inject.Singleton;
-import java.util.ArrayList;
import java.util.Collection;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-@Singleton
-public class ReviewerManager {
- private static final Logger log = LoggerFactory.getLogger(ReviewerManager.class);
+public interface ReviewerManager {
- private final OneOffRequestContext requestContext;
- private final GerritApi gApi;
- private final IdentifiedUser.GenericFactory userFactory;
- private final ChangeData.Factory changeDataFactory;
- private final PermissionBackend permissionBackend;
-
- @Inject
- public ReviewerManager(
- OneOffRequestContext requestContext,
- GerritApi gApi,
- IdentifiedUser.GenericFactory userFactory,
- ChangeData.Factory changeDataFactory,
- PermissionBackend permissionBackend) {
- this.requestContext = requestContext;
- this.gApi = gApi;
- this.userFactory = userFactory;
- this.changeDataFactory = changeDataFactory;
- this.permissionBackend = permissionBackend;
- }
-
- public void addReviewers(ChangeApi cApi, Collection<Account.Id> reviewers)
- throws ReviewerManagerException {
- try {
- ChangeInfo changeInfo = cApi.get();
- try (ManualRequestContext ctx =
- requestContext.openAs(new Account.Id(changeInfo.owner._accountId))) {
- // TODO(davido): Switch back to using changes API again,
- // when it supports batch mode for adding reviewers
- ReviewInput in = new ReviewInput();
- in.reviewers = new ArrayList<>(reviewers.size());
- for (Account.Id account : reviewers) {
- if (isVisibleTo(ctx.getReviewDbProvider().get(), changeInfo, account)) {
- AddReviewerInput addReviewerInput = new AddReviewerInput();
- addReviewerInput.reviewer = account.toString();
- in.reviewers.add(addReviewerInput);
- } else {
- log.warn(
- "Not adding account {} as reviewer to change {} because the associated ref is not visible",
- account,
- changeInfo._number);
- }
- }
- gApi.changes().id(changeInfo.id).current().review(in);
- }
- } catch (RestApiException | OrmException e) {
- log.error("Couldn't add reviewers to the change", e);
- throw new ReviewerManagerException(e);
- }
- }
-
- private boolean isVisibleTo(ReviewDb reviewDb, ChangeInfo changeInfo, Id account) {
- ChangeData changeData =
- changeDataFactory.create(
- reviewDb, new Project.NameKey(changeInfo.project), new Change.Id(changeInfo._number));
- return permissionBackend
- .user(userFactory.create(account))
- .change(changeData)
- .testOrFalse(ChangePermission.READ);
- }
+ void addReviewers(ChangeApi cApi, Collection<Account.Id> reviewers)
+ throws ReviewerManagerException;
}
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/SyncReviewerManager.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/SyncReviewerManager.java
new file mode 100644
index 0000000..e22446d
--- /dev/null
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/SyncReviewerManager.java
@@ -0,0 +1,108 @@
+// Copyright (c) 2013 VMware, Inc. All Rights Reserved.
+// Copyright (C) 2017 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.gerrit.extensions.api.GerritApi;
+import com.google.gerrit.extensions.api.changes.AddReviewerInput;
+import com.google.gerrit.extensions.api.changes.ChangeApi;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Account.Id;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.util.ManualRequestContext;
+import com.google.gerrit.server.util.OneOffRequestContext;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.util.ArrayList;
+import java.util.Collection;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Singleton
+class SyncReviewerManager implements ReviewerManager {
+ private static final Logger log = LoggerFactory.getLogger(SyncReviewerManager.class);
+
+ private final OneOffRequestContext requestContext;
+ private final GerritApi gApi;
+ private final IdentifiedUser.GenericFactory userFactory;
+ private final ChangeData.Factory changeDataFactory;
+ private final PermissionBackend permissionBackend;
+
+ @Inject
+ public SyncReviewerManager(
+ OneOffRequestContext requestContext,
+ GerritApi gApi,
+ IdentifiedUser.GenericFactory userFactory,
+ ChangeData.Factory changeDataFactory,
+ PermissionBackend permissionBackend) {
+ this.requestContext = requestContext;
+ this.gApi = gApi;
+ this.userFactory = userFactory;
+ this.changeDataFactory = changeDataFactory;
+ this.permissionBackend = permissionBackend;
+ }
+
+ @Override
+ public void addReviewers(ChangeApi cApi, Collection<Account.Id> reviewers)
+ throws ReviewerManagerException {
+ try {
+ ChangeInfo changeInfo = cApi.get();
+ try (ManualRequestContext ctx =
+ requestContext.openAs(new Account.Id(changeInfo.owner._accountId))) {
+ // TODO(davido): Switch back to using changes API again,
+ // when it supports batch mode for adding reviewers
+ ReviewInput in = new ReviewInput();
+ in.reviewers = new ArrayList<>(reviewers.size());
+ for (Account.Id account : reviewers) {
+ if (isVisibleTo(ctx.getReviewDbProvider().get(), changeInfo, account)) {
+ AddReviewerInput addReviewerInput = new AddReviewerInput();
+ addReviewerInput.reviewer = account.toString();
+ in.reviewers.add(addReviewerInput);
+ } else {
+ log.warn(
+ "Not adding account {} as reviewer to change {} because the associated ref is not visible",
+ account,
+ changeInfo._number);
+ }
+ }
+ gApi.changes().id(changeInfo.id).current().review(in);
+ }
+ } catch (RestApiException | OrmException e) {
+ log.error("Couldn't add reviewers to the change", e);
+ throw new ReviewerManagerException(e);
+ }
+ }
+
+ private boolean isVisibleTo(ReviewDb reviewDb, ChangeInfo changeInfo, Id account) {
+ ChangeData changeData =
+ changeDataFactory.create(
+ reviewDb, new Project.NameKey(changeInfo.project), new Change.Id(changeInfo._number));
+ return permissionBackend
+ .user(userFactory.create(account))
+ .change(changeData)
+ .testOrFalse(ChangePermission.READ);
+ }
+}
diff --git a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerIT.java b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerIT.java
similarity index 94%
rename from owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerIT.java
rename to owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerIT.java
index f009bde..88216a7 100644
--- a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerIT.java
+++ b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerIT.java
@@ -13,7 +13,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-package com.vmware.gerrit.owners.common;
+package com.googlesource.gerrit.owners.common;
import static org.junit.Assert.assertEquals;
@@ -28,12 +28,14 @@
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
+import com.googlesource.gerrit.owners.common.ReviewerManager;
+
import org.eclipse.jgit.transport.ReceiveCommand.Type;
import org.junit.Test;
@TestPlugin(
name = "owners-autoassign",
- sysModule = "com.vmware.gerrit.owners.common.GitRefListenerIT$TestModule")
+ sysModule = "com.googlesource.gerrit.owners.common.GitRefListenerIT$TestModule")
public class GitRefListenerIT extends LightweightPluginDaemonTest {
@Inject GitRefListenerTest gitRefListener;
diff --git a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerTest.java
similarity index 95%
rename from owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java
rename to owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerTest.java
index e3d4be4..e862ed9 100644
--- a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java
+++ b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerTest.java
@@ -13,7 +13,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-package com.vmware.gerrit.owners.common;
+package com.googlesource.gerrit.owners.common;
import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.server.CurrentUser;
@@ -38,7 +38,7 @@
PatchListCache patchListCache,
GitRepositoryManager repositoryManager,
Accounts accounts,
- ReviewerManager reviewerManager,
+ SyncReviewerManager reviewerManager,
OneOffRequestContext oneOffReqCtx,
Provider<CurrentUser> currentUserProvider) {
super(
diff --git a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/ReferenceUpdatedEventTest.java b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/ReferenceUpdatedEventTest.java
similarity index 97%
rename from owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/ReferenceUpdatedEventTest.java
rename to owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/ReferenceUpdatedEventTest.java
index 9946502..42ac39c 100644
--- a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/ReferenceUpdatedEventTest.java
+++ b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/ReferenceUpdatedEventTest.java
@@ -13,7 +13,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-package com.vmware.gerrit.owners.common;
+package com.googlesource.gerrit.owners.common;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.common.AccountInfo;
