From 676a0155e05bb8d029b43cacf97a3a5d234cfaaa Mon Sep 17 00:00:00 2001 From: Yuxuan 'fishy' Wang Date: Tue, 23 Aug 2016 16:55:26 -0700 Subject: [PATCH] Add batch mode to Abandon When an extension wants to abandon more than one changes in the same project, batch them together inside a BatchUpdate will be much better than calling Abandon REST API multiple times, so this change provides the batch mode. Also changed AbandonUtil to group changes by project and use the new batchAbandon code. Change-Id: I9d89b58d4cfa469b666a234af1a564c1f5211b19 --- .../gerrit/acceptance/AbstractDaemonTest.java | 4 + .../acceptance/api/change/ChangeIT.java | 60 ++++++++++++++ .../google/gerrit/server/change/Abandon.java | 78 ++++++++++++++++--- .../gerrit/server/change/AbandonUtil.java | 53 ++++++++----- 4 files changed, 164 insertions(+), 31 deletions(-) diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 79ff53a66b..5530be3ff3 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -64,6 +64,7 @@ import com.google.gerrit.server.OutputFormat; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.GroupCache; +import com.google.gerrit.server.change.Abandon; import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.change.Revisions; @@ -238,6 +239,9 @@ public abstract class AbstractDaemonTest { @Inject protected ChangeNotes.Factory notesFactory; + @Inject + protected Abandon changeAbandoner; + @Rule public ExpectedException exception = ExpectedException.none(); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index 85af4240bd..52cdc5c662 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -31,6 +31,7 @@ import static java.util.concurrent.TimeUnit.SECONDS; import static org.junit.Assert.fail; import com.google.common.base.Function; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.AbstractDaemonTest; @@ -73,6 +74,7 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.config.AnonymousCowardNameProvider; import com.google.gerrit.server.git.ProjectConfig; @@ -184,6 +186,64 @@ public class ChangeIT extends AbstractDaemonTest { .abandon(); } + @Test + public void batchAbandon() throws Exception { + CurrentUser user = atrScope.get().getUser(); + PushOneCommit.Result a = createChange(); + List controlA = changeFinder.find(a.getChangeId(), user); + assertThat(controlA).hasSize(1); + PushOneCommit.Result b = createChange(); + List controlB = changeFinder.find(b.getChangeId(), user); + assertThat(controlB).hasSize(1); + List list = + ImmutableList.of(controlA.get(0), controlB.get(0)); + changeAbandoner.batchAbandon( + controlA.get(0).getProject().getNameKey(), user, list, "deadbeef"); + + ChangeInfo info = get(a.getChangeId()); + assertThat(info.status).isEqualTo(ChangeStatus.ABANDONED); + assertThat(Iterables.getLast(info.messages).message.toLowerCase()) + .contains("abandoned"); + assertThat(Iterables.getLast(info.messages).message.toLowerCase()) + .contains("deadbeef"); + + info = get(b.getChangeId()); + assertThat(info.status).isEqualTo(ChangeStatus.ABANDONED); + assertThat(Iterables.getLast(info.messages).message.toLowerCase()) + .contains("abandoned"); + assertThat(Iterables.getLast(info.messages).message.toLowerCase()) + .contains("deadbeef"); + } + + @Test + public void batchAbandonChangeProject() throws Exception { + String project1Name = name("Project1"); + String project2Name = name("Project2"); + gApi.projects().create(project1Name); + gApi.projects().create(project2Name); + TestRepository project1 = + cloneProject(new Project.NameKey(project1Name)); + TestRepository project2 = + cloneProject(new Project.NameKey(project2Name)); + + CurrentUser user = atrScope.get().getUser(); + PushOneCommit.Result a = + createChange(project1, "master", "x", "x", "x", ""); + List controlA = changeFinder.find(a.getChangeId(), user); + assertThat(controlA).hasSize(1); + PushOneCommit.Result b = + createChange(project2, "master", "x", "x", "x", ""); + List controlB = changeFinder.find(b.getChangeId(), user); + assertThat(controlB).hasSize(1); + List list = + ImmutableList.of(controlA.get(0), controlB.get(0)); + exception.expect(ResourceConflictException.class); + exception.expectMessage(String.format( + "Project name \"%s\" doesn't match \"%s\"", + project2Name, project1Name)); + changeAbandoner.batchAbandon(new Project.NameKey(project1Name), user, list); + } + @Test public void abandonDraft() throws Exception { PushOneCommit.Result r = createDraftChange(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java index adbcf22c29..a15915a360 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java @@ -28,6 +28,7 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.ChangeUtil; @@ -46,9 +47,9 @@ import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; - import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.util.Collection; @Singleton public class Abandon implements RestModifyView, @@ -91,6 +92,11 @@ public class Abandon implements RestModifyView, return json.create(ChangeJson.NO_OPTIONS).format(change); } + public Change abandon(ChangeControl control) + throws RestApiException, UpdateException { + return abandon(control, "", NotifyHandling.ALL); + } + public Change abandon(ChangeControl control, String msgTxt) throws RestApiException, UpdateException { return abandon(control, msgTxt, NotifyHandling.ALL); @@ -98,31 +104,79 @@ public class Abandon implements RestModifyView, public Change abandon(ChangeControl control, String msgTxt, NotifyHandling notifyHandling) throws RestApiException, UpdateException { - CurrentUser user = control.getUser(); - Account account = user.isIdentifiedUser() - ? user.asIdentifiedUser().getAccount() - : null; - Op op = new Op(msgTxt, account, notifyHandling); - try (BatchUpdate u = batchUpdateFactory.create(dbProvider.get(), - control.getProject().getNameKey(), user, TimeUtil.nowTs())) { + Op op = new Op(control, msgTxt, notifyHandling); + try (BatchUpdate u = + batchUpdateFactory.create( + dbProvider.get(), + control.getProject().getNameKey(), + control.getUser(), + TimeUtil.nowTs())) { u.addOp(control.getId(), op).execute(); } return op.change; } + /** + * If an extension has more than one changes to abandon that belong to the + * same project, they should use the batch instead of abandoning one by one. + *

+ * It's the caller's responsibility to ensure that all jobs inside the same + * batch have the matching project from its ChangeControl. Violations will + * result in a ResourceConflictException. + */ + public void batchAbandon(Project.NameKey project, CurrentUser user, + Collection controls, String msgTxt, + NotifyHandling notifyHandling) throws RestApiException, UpdateException { + if (controls.isEmpty()) { + return; + } + try (BatchUpdate u = batchUpdateFactory.create( + dbProvider.get(), project, user, TimeUtil.nowTs())) { + for (ChangeControl control : controls) { + if (!project.equals(control.getProject().getNameKey())) { + throw new ResourceConflictException( + String.format( + "Project name \"%s\" doesn't match \"%s\"", + control.getProject().getNameKey().get(), + project.get())); + } + u.addOp(control.getId(), new Op(control, msgTxt, notifyHandling)); + } + u.execute(); + } + } + + public void batchAbandon(Project.NameKey project, CurrentUser user, + Collection controls, String msgTxt) + throws RestApiException, UpdateException { + batchAbandon(project, user, controls, msgTxt, NotifyHandling.ALL); + } + + public void batchAbandon(Project.NameKey project, CurrentUser user, + Collection controls) + throws RestApiException, UpdateException { + batchAbandon(project, user, controls, "", NotifyHandling.ALL); + } + private class Op extends BatchUpdate.Op { - private final Account account; + private final ChangeControl control; private final String msgTxt; + private final NotifyHandling notifyHandling; + private final Account account; private Change change; private PatchSet patchSet; private ChangeMessage message; - private NotifyHandling notifyHandling; - private Op(String msgTxt, Account account, NotifyHandling notifyHandling) { - this.account = account; + private Op(ChangeControl control, String msgTxt, + NotifyHandling notifyHandling) { + this.control = control; this.msgTxt = msgTxt; this.notifyHandling = notifyHandling; + CurrentUser user = control.getUser(); + account = user.isIdentifiedUser() + ? user.asIdentifiedUser().getAccount() + : null; } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java index f84599d85d..28e84be045 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java @@ -14,7 +14,10 @@ package com.google.gerrit.server.change; +import com.google.common.collect.ImmutableMultimap; +import com.google.common.collect.Multimap; import com.google.gerrit.extensions.restapi.ResourceConflictException; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.InternalUser; import com.google.gerrit.server.config.ChangeCleanupConfig; import com.google.gerrit.server.project.ChangeControl; @@ -25,10 +28,9 @@ import com.google.gerrit.server.query.change.ChangeQueryProcessor; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Singleton; - import org.slf4j.Logger; import org.slf4j.LoggerFactory; - +import java.util.Collection; import java.util.List; import java.util.concurrent.TimeUnit; @@ -37,10 +39,10 @@ public class AbandonUtil { private static final Logger log = LoggerFactory.getLogger(AbandonUtil.class); private final ChangeCleanupConfig cfg; - private final InternalUser.Factory internalUserFactory; private final ChangeQueryProcessor queryProcessor; private final ChangeQueryBuilder queryBuilder; private final Abandon abandon; + private final InternalUser internalUser; @Inject AbandonUtil( @@ -50,10 +52,10 @@ public class AbandonUtil { ChangeQueryBuilder queryBuilder, Abandon abandon) { this.cfg = cfg; - this.internalUserFactory = internalUserFactory; this.queryProcessor = queryProcessor; this.queryBuilder = queryBuilder; this.abandon = abandon; + internalUser = internalUserFactory.create(); } public void abandonInactiveOpenChanges() { @@ -68,29 +70,42 @@ public class AbandonUtil { if (!cfg.getAbandonIfMergeable()) { query += " -is:mergeable"; } - List changesToAbandon = queryProcessor.enforceVisibility(false) - .query(queryBuilder.parse(query)).entities(); - int count = 0; + + List changesToAbandon = + queryProcessor + .enforceVisibility(false) + .query(queryBuilder.parse(query)) + .entities(); + ImmutableMultimap.Builder builder = + ImmutableMultimap.builder(); for (ChangeData cd : changesToAbandon) { + ChangeControl control = cd.changeControl(internalUser); + builder.put(control.getProject().getNameKey(), control); + } + + int count = 0; + Multimap abandons = builder.build(); + String message = cfg.getAbandonMessage(); + for (Project.NameKey project : abandons.keySet()) { + Collection changes = abandons.get(project); try { - abandon.abandon(changeControl(cd), cfg.getAbandonMessage()); - count++; - } catch (ResourceConflictException e) { - // Change was already merged or abandoned. + abandon.batchAbandon(project, internalUser, changes, message); + count += changes.size(); } catch (Throwable e) { - log.error(String.format( - "Failed to auto-abandon inactive open change %d.", - cd.getId().get()), e); + StringBuilder msg = + new StringBuilder("Failed to auto-abandon inactive change(s):"); + for (ChangeControl change : changes) { + msg.append(" ").append(change.getId().get()); + } + msg.append("."); + log.error(msg.toString(), e); } } log.info(String.format("Auto-Abandoned %d of %d changes.", count, changesToAbandon.size())); } catch (QueryParseException | OrmException e) { - log.error("Failed to query inactive open changes for auto-abandoning.", e); + log.error( + "Failed to query inactive open changes for auto-abandoning.", e); } } - - private ChangeControl changeControl(ChangeData cd) throws OrmException { - return cd.changeControl(internalUserFactory.create()); - } }