Run /abandon and /restore in a single database operation
On the backend behind gerrit-review each database call is expensive,
unless we do something crazy like wrap a block of database operations
into a single mock transaction with the beginTransaction/commit idiom.
Simplify both of these tasks by getting rid of the ugly helper
function from ChangeUtil and explicitly perform all database
operations before sending email to interested parties.
Change-Id: I63cbbc7ac47d20f202b042740abb204e040e7c65
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
index d7a487d..afbfd8d3 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
@@ -48,7 +48,7 @@
private final ApprovalTypes approvalTypes;
@Inject
- ApprovalsUtil(ReviewDb db, ApprovalTypes approvalTypes) {
+ public ApprovalsUtil(ReviewDb db, ApprovalTypes approvalTypes) {
this.db = db;
this.approvalTypes = approvalTypes;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
index 76ed67e..920299d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
@@ -30,7 +30,6 @@
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MergeOp;
import com.google.gerrit.server.mail.EmailException;
-import com.google.gerrit.server.mail.ReplyToChangeSender;
import com.google.gerrit.server.mail.RevertedSender;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
@@ -56,8 +55,6 @@
import org.eclipse.jgit.util.Base64;
import org.eclipse.jgit.util.ChangeIdUtil;
import org.eclipse.jgit.util.NB;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.sql.Timestamp;
@@ -70,8 +67,6 @@
import java.util.regex.Matcher;
public class ChangeUtil {
- private static final Logger log = LoggerFactory.getLogger(ChangeUtil.class);
-
private static int uuidPrefix;
private static int uuidSeq;
@@ -513,25 +508,6 @@
db.patchSets().delete(Collections.singleton(patch));
}
- public static <T extends ReplyToChangeSender> void updatedChange(
- final ReviewDb db, final IdentifiedUser user, final Change change,
- final ChangeMessage cmsg, ReplyToChangeSender.Factory<T> senderFactory)
- throws OrmException {
- db.changeMessages().insert(Collections.singleton(cmsg));
-
- new ApprovalsUtil(db, null).syncChangeStatus(change);
-
- // Email the reviewers
- try {
- final ReplyToChangeSender cm = senderFactory.create(change);
- cm.setFrom(user.getAccountId());
- cm.setChangeMessage(cmsg);
- cm.send();
- } catch (Exception e) {
- log.error("Cannot email update for change " + change.getChangeId(), e);
- }
- }
-
public static String sortKey(long lastUpdated, int id){
// The encoding uses minutes since Wed Oct 1 00:00:00 2008 UTC.
// We overrun approximately 4,085 years later, so ~6093.
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 1249e9b..3dc003b 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
@@ -23,18 +23,27 @@
import com.google.gerrit.extensions.restapi.RestModifyView;
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.server.ReviewDb;
+import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.Abandon.Input;
import com.google.gerrit.server.mail.AbandonedSender;
+import com.google.gerrit.server.mail.ReplyToChangeSender;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gwtorm.server.AtomicUpdate;
+import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collections;
+
public class Abandon implements RestModifyView<ChangeResource, Input> {
+ private static final Logger log = LoggerFactory.getLogger(Abandon.class);
+
private final ChangeHooks hooks;
private final AbandonedSender.Factory abandonedSenderFactory;
private final Provider<ReviewDb> dbProvider;
@@ -66,6 +75,7 @@
throws BadRequestException, AuthException,
ResourceConflictException, Exception {
ChangeControl control = req.getControl();
+ IdentifiedUser caller = (IdentifiedUser) control.getCurrentUser();
Change change = req.getChange();
if (!control.canAbandon()) {
throw new AuthException("abandon not permitted");
@@ -73,46 +83,68 @@
throw new ResourceConflictException("change is " + status(change));
}
- // Create a message to accompany the abandoned change
+ ChangeMessage message;
ReviewDb db = dbProvider.get();
- PatchSet.Id patchSetId = change.currentPatchSetId();
- IdentifiedUser currentUser = (IdentifiedUser) control.getCurrentUser();
- String message = Strings.emptyToNull(input.message);
- ChangeMessage cmsg = new ChangeMessage(
- new ChangeMessage.Key(change.getId(), ChangeUtil.messageUUID(db)),
- currentUser.getAccountId(), patchSetId);
- StringBuilder msg = new StringBuilder();
- msg.append(String.format("Patch Set %d: Abandoned", patchSetId.get()));
- if (message != null) {
- msg.append("\n\n");
- msg.append(message);
- }
- cmsg.setMessage(msg.toString());
-
- // Abandon the change
- Change updatedChange = db.changes().atomicUpdate(
- change.getId(),
- new AtomicUpdate<Change>() {
- @Override
- public Change update(Change change) {
- if (change.getStatus().isOpen()) {
- change.setStatus(Change.Status.ABANDONED);
- ChangeUtil.updated(change);
- return change;
+ db.changes().beginTransaction(change.getId());
+ try {
+ change = db.changes().atomicUpdate(
+ change.getId(),
+ new AtomicUpdate<Change>() {
+ @Override
+ public Change update(Change change) {
+ if (change.getStatus().isOpen()) {
+ change.setStatus(Change.Status.ABANDONED);
+ ChangeUtil.updated(change);
+ return change;
+ }
+ return null;
}
- return null;
- }
- });
- if (updatedChange == null) {
- throw new ResourceConflictException("change is "
- + status(db.changes().get(change.getId())));
+ });
+ if (change == null) {
+ throw new ResourceConflictException("change is "
+ + status(db.changes().get(req.getChange().getId())));
+ }
+ message = newMessage(input, caller, change);
+ db.changeMessages().insert(Collections.singleton(message));
+ new ApprovalsUtil(db, null).syncChangeStatus(change);
+ db.commit();
+ } finally {
+ db.rollback();
}
- ChangeUtil.updatedChange(db, currentUser, updatedChange, cmsg,
- abandonedSenderFactory);
- hooks.doChangeAbandonedHook(updatedChange, currentUser.getAccount(),
- message, db);
- return json.format(change.getId());
+ try {
+ ReplyToChangeSender cm = abandonedSenderFactory.create(change);
+ cm.setFrom(caller.getAccountId());
+ cm.setChangeMessage(message);
+ cm.send();
+ } catch (Exception e) {
+ log.error("Cannot email update for change " + change.getChangeId(), e);
+ }
+ hooks.doChangeAbandonedHook(change,
+ caller.getAccount(),
+ Strings.emptyToNull(input.message),
+ db);
+ return json.format(change);
+ }
+
+ private ChangeMessage newMessage(Input input, IdentifiedUser caller,
+ Change change) throws OrmException {
+ StringBuilder msg = new StringBuilder();
+ msg.append("Abandoned");
+ if (!Strings.nullToEmpty(input.message).trim().isEmpty()) {
+ msg.append("\n\n");
+ msg.append(input.message.trim());
+ }
+
+ ChangeMessage message = new ChangeMessage(
+ new ChangeMessage.Key(
+ change.getId(),
+ ChangeUtil.messageUUID(dbProvider.get())),
+ caller.getAccountId(),
+ change.getLastUpdatedOn(),
+ change.currentPatchSetId());
+ message.setMessage(msg.toString());
+ return message;
}
private static String status(Change change) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java
index 61e01db..5b78b5b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java
@@ -23,18 +23,27 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Change.Status;
import com.google.gerrit.reviewdb.client.ChangeMessage;
-import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.Restore.Input;
+import com.google.gerrit.server.mail.ReplyToChangeSender;
import com.google.gerrit.server.mail.RestoredSender;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gwtorm.server.AtomicUpdate;
+import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collections;
+
public class Restore implements RestModifyView<ChangeResource, Input> {
+ private static final Logger log = LoggerFactory.getLogger(Restore.class);
+
private final ChangeHooks hooks;
private final RestoredSender.Factory restoredSenderFactory;
private final Provider<ReviewDb> dbProvider;
@@ -65,6 +74,7 @@
public Object apply(ChangeResource req, Input input)
throws Exception {
ChangeControl control = req.getControl();
+ IdentifiedUser caller = (IdentifiedUser) control.getCurrentUser();
Change change = req.getChange();
if (!control.canRestore()) {
throw new AuthException("restore not permitted");
@@ -72,46 +82,68 @@
throw new ResourceConflictException("change is " + status(change));
}
- // Create a message to accompany the restore change
+ ChangeMessage message;
ReviewDb db = dbProvider.get();
- PatchSet.Id patchSetId = change.currentPatchSetId();
- IdentifiedUser currentUser = (IdentifiedUser) control.getCurrentUser();
- String message = Strings.emptyToNull(input.message);
- final ChangeMessage cmsg = new ChangeMessage(
- new ChangeMessage.Key(change.getId(), ChangeUtil.messageUUID(db)),
- currentUser.getAccountId(), patchSetId);
- StringBuilder msg = new StringBuilder();
- msg.append(String.format("Patch Set %d: Restored", patchSetId.get()));
- if (message != null) {
- msg.append("\n\n");
- msg.append(message);
- }
- cmsg.setMessage(msg.toString());
-
- // Restore the change
- final Change updatedChange = db.changes().atomicUpdate(
- change.getId(),
- new AtomicUpdate<Change>() {
- @Override
- public Change update(Change change) {
- if (change.getStatus() == Change.Status.ABANDONED) {
- change.setStatus(Change.Status.NEW);
- ChangeUtil.updated(change);
- return change;
+ db.changes().beginTransaction(change.getId());
+ try {
+ change = db.changes().atomicUpdate(
+ change.getId(),
+ new AtomicUpdate<Change>() {
+ @Override
+ public Change update(Change change) {
+ if (change.getStatus() == Change.Status.ABANDONED) {
+ change.setStatus(Change.Status.NEW);
+ ChangeUtil.updated(change);
+ return change;
+ }
+ return null;
}
- return null;
- }
- });
- if (updatedChange == null) {
- throw new ResourceConflictException("change is "
- + status(db.changes().get(change.getId())));
+ });
+ if (change == null) {
+ throw new ResourceConflictException("change is "
+ + status(db.changes().get(req.getChange().getId())));
+ }
+ message = newMessage(input, caller, change);
+ db.changeMessages().insert(Collections.singleton(message));
+ new ApprovalsUtil(db, null).syncChangeStatus(change);
+ db.commit();
+ } finally {
+ db.rollback();
}
- ChangeUtil.updatedChange(db, currentUser, updatedChange, cmsg,
- restoredSenderFactory);
- hooks.doChangeRestoredHook(updatedChange, currentUser.getAccount(),
- message, db);
- return json.format(change.getId());
+ try {
+ ReplyToChangeSender cm = restoredSenderFactory.create(change);
+ cm.setFrom(caller.getAccountId());
+ cm.setChangeMessage(message);
+ cm.send();
+ } catch (Exception e) {
+ log.error("Cannot email update for change " + change.getChangeId(), e);
+ }
+ hooks.doChangeRestoredHook(change,
+ caller.getAccount(),
+ Strings.emptyToNull(input.message),
+ dbProvider.get());
+ return json.format(change);
+ }
+
+ private ChangeMessage newMessage(Input input, IdentifiedUser caller,
+ Change change) throws OrmException {
+ StringBuilder msg = new StringBuilder();
+ msg.append("Restored");
+ if (!Strings.nullToEmpty(input.message).trim().isEmpty()) {
+ msg.append("\n\n");
+ msg.append(input.message.trim());
+ }
+
+ ChangeMessage message = new ChangeMessage(
+ new ChangeMessage.Key(
+ change.getId(),
+ ChangeUtil.messageUUID(dbProvider.get())),
+ caller.getAccountId(),
+ change.getLastUpdatedOn(),
+ change.currentPatchSetId());
+ message.setMessage(msg.toString());
+ return message;
}
private static String status(Change change) {
