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;