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
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 79ff53a..5530be3 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.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 @@
@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 85af424..52cdc5c 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 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.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;
@@ -185,6 +187,64 @@
}
@Test
+ public void batchAbandon() throws Exception {
+ CurrentUser user = atrScope.get().getUser();
+ PushOneCommit.Result a = createChange();
+ List<ChangeControl> controlA = changeFinder.find(a.getChangeId(), user);
+ assertThat(controlA).hasSize(1);
+ PushOneCommit.Result b = createChange();
+ List<ChangeControl> controlB = changeFinder.find(b.getChangeId(), user);
+ assertThat(controlB).hasSize(1);
+ List<ChangeControl> 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<InMemoryRepository> project1 =
+ cloneProject(new Project.NameKey(project1Name));
+ TestRepository<InMemoryRepository> project2 =
+ cloneProject(new Project.NameKey(project2Name));
+
+ CurrentUser user = atrScope.get().getUser();
+ PushOneCommit.Result a =
+ createChange(project1, "master", "x", "x", "x", "");
+ List<ChangeControl> controlA = changeFinder.find(a.getChangeId(), user);
+ assertThat(controlA).hasSize(1);
+ PushOneCommit.Result b =
+ createChange(project2, "master", "x", "x", "x", "");
+ List<ChangeControl> controlB = changeFinder.find(b.getChangeId(), user);
+ assertThat(controlB).hasSize(1);
+ List<ChangeControl> 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();
String changeId = r.getChangeId();
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 adbcf22..a15915a 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.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.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<ChangeResource, AbandonInput>,
@@ -91,6 +92,11 @@
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 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.
+ * <p>
+ * 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<ChangeControl> 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<ChangeControl> controls, String msgTxt)
+ throws RestApiException, UpdateException {
+ batchAbandon(project, user, controls, msgTxt, NotifyHandling.ALL);
+ }
+
+ public void batchAbandon(Project.NameKey project, CurrentUser user,
+ Collection<ChangeControl> 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 f84599d..28e84be 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.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 @@
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 @@
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 @@
if (!cfg.getAbandonIfMergeable()) {
query += " -is:mergeable";
}
- List<ChangeData> changesToAbandon = queryProcessor.enforceVisibility(false)
- .query(queryBuilder.parse(query)).entities();
- int count = 0;
+
+ List<ChangeData> changesToAbandon =
+ queryProcessor
+ .enforceVisibility(false)
+ .query(queryBuilder.parse(query))
+ .entities();
+ ImmutableMultimap.Builder<Project.NameKey, ChangeControl> builder =
+ ImmutableMultimap.builder();
for (ChangeData cd : changesToAbandon) {
+ ChangeControl control = cd.changeControl(internalUser);
+ builder.put(control.getProject().getNameKey(), control);
+ }
+
+ int count = 0;
+ Multimap<Project.NameKey, ChangeControl> abandons = builder.build();
+ String message = cfg.getAbandonMessage();
+ for (Project.NameKey project : abandons.keySet()) {
+ Collection<ChangeControl> 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());
- }
}
