Use transactions to handle comments when possible

If the database supports transactions (see gwtorm, not all do)
try to perform some update operations as a single transaction on
the change. This may allow the database to batch together any
record updates, saving some time.

Change-Id: I5af9efe3a541d83515109e3bf9a3497b3d8127de

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java

@@ -110,18 +110,25 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
       final AsyncCallback<VoidResult> callback) {
     run(callback, new Action<VoidResult>() {
       public VoidResult run(ReviewDb db) throws OrmException, Failure {
-        final PatchLineComment comment = db.patchComments().get(commentKey);
+        Change.Id id = commentKey.getParentKey().getParentKey().getParentKey();
-        if (comment == null) {
+        db.changes().beginTransaction(id);
-          throw new Failure(new NoSuchEntityException());
+        try {
+          final PatchLineComment comment = db.patchComments().get(commentKey);
+          if (comment == null) {
+            throw new Failure(new NoSuchEntityException());
+          }
+          if (!getAccountId().equals(comment.getAuthor())) {
+            throw new Failure(new NoSuchEntityException());
+          }
+          if (comment.getStatus() != PatchLineComment.Status.DRAFT) {
+            throw new Failure(new IllegalStateException("Comment published"));
+          }
+          db.patchComments().delete(Collections.singleton(comment));
+          db.commit();
+          return VoidResult.INSTANCE;
+        } finally {
+          db.rollback();
         }
-        if (!getAccountId().equals(comment.getAuthor())) {
-          throw new Failure(new NoSuchEntityException());
-        }
-        if (comment.getStatus() != PatchLineComment.Status.DRAFT) {
-          throw new Failure(new IllegalStateException("Comment published"));
-        }
-        db.patchComments().delete(Collections.singleton(comment));
-        return VoidResult.INSTANCE;
       }
     });
   }
@@ -142,14 +149,20 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
         Account.Id account = getAccountId();
         AccountPatchReview.Key key =
             new AccountPatchReview.Key(patchKey, account);
-        AccountPatchReview apr = db.accountPatchReviews().get(key);
+        db.accounts().beginTransaction(account);
-        if (apr == null && reviewed) {
+        try {
-          db.accountPatchReviews().insert(
+          AccountPatchReview apr = db.accountPatchReviews().get(key);
-              Collections.singleton(new AccountPatchReview(patchKey, account)));
+          if (apr == null && reviewed) {
-        } else if (apr != null && !reviewed) {
+            db.accountPatchReviews().insert(
-          db.accountPatchReviews().delete(Collections.singleton(apr));
+                Collections.singleton(new AccountPatchReview(patchKey, account)));
+          } else if (apr != null && !reviewed) {
+            db.accountPatchReviews().delete(Collections.singleton(apr));
+          }
+          db.commit();
+          return VoidResult.INSTANCE;
+        } finally {
+          db.rollback();
         }
-        return VoidResult.INSTANCE;
       }
     });
   }

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/SaveDraft.java

@@ -49,7 +49,6 @@ class SaveDraft extends Handler<PatchLineComment> {
     this.changeControlFactory = changeControlFactory;
     this.db = db;
     this.currentUser = currentUser;
-
     this.comment = comment;
   }
 
@@ -62,42 +61,50 @@ class SaveDraft extends Handler<PatchLineComment> {
     final Patch.Key patchKey = comment.getKey().getParentKey();
     final PatchSet.Id patchSetId = patchKey.getParentKey();
     final Change.Id changeId = patchKey.getParentKey().getParentKey();
-    changeControlFactory.validateFor(changeId);
-    if (db.patchSets().get(patchSetId) == null) {
-      throw new NoSuchChangeException(changeId);
-    }
 
-    final Account.Id me = currentUser.getAccountId();
+    db.changes().beginTransaction(changeId);
-    if (comment.getKey().get() == null) {
+    try {
-      if (comment.getLine() < 1) {
+      changeControlFactory.validateFor(changeId);
-        throw new IllegalStateException("Comment line must be >= 1, not "
+      if (db.patchSets().get(patchSetId) == null) {
-            + comment.getLine());
-      }
-
-      if (comment.getParentUuid() != null) {
-        final PatchLineComment parent =
-            db.patchComments().get(
-                new PatchLineComment.Key(patchKey, comment.getParentUuid()));
-        if (parent == null || parent.getSide() != comment.getSide()) {
-          throw new IllegalStateException("Parent comment must be on same side");
-        }
-      }
-
-      final PatchLineComment nc =
-          new PatchLineComment(new PatchLineComment.Key(patchKey, ChangeUtil
-              .messageUUID(db)), comment.getLine(), me, comment.getParentUuid());
-      nc.setSide(comment.getSide());
-      nc.setMessage(comment.getMessage());
-      db.patchComments().insert(Collections.singleton(nc));
-      return nc;
-
-    } else {
-      if (!me.equals(comment.getAuthor())) {
         throw new NoSuchChangeException(changeId);
       }
-      comment.updated();
+
-      db.patchComments().update(Collections.singleton(comment));
+      final Account.Id me = currentUser.getAccountId();
-      return comment;
+      if (comment.getKey().get() == null) {
+        if (comment.getLine() < 1) {
+          throw new IllegalStateException("Comment line must be >= 1, not "
+              + comment.getLine());
+        }
+
+        if (comment.getParentUuid() != null) {
+          final PatchLineComment parent =
+              db.patchComments().get(
+                  new PatchLineComment.Key(patchKey, comment.getParentUuid()));
+          if (parent == null || parent.getSide() != comment.getSide()) {
+            throw new IllegalStateException("Parent comment must be on same side");
+          }
+        }
+
+        final PatchLineComment nc =
+            new PatchLineComment(new PatchLineComment.Key(patchKey, ChangeUtil
+                .messageUUID(db)), comment.getLine(), me, comment.getParentUuid());
+        nc.setSide(comment.getSide());
+        nc.setMessage(comment.getMessage());
+        db.patchComments().insert(Collections.singleton(nc));
+        db.commit();
+        return nc;
+
+      } else {
+        if (!me.equals(comment.getAuthor())) {
+          throw new NoSuchChangeException(changeId);
+        }
+        comment.updated();
+        db.patchComments().update(Collections.singleton(comment));
+        db.commit();
+        return comment;
+      }
+    } finally {
+      db.rollback();
     }
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java

@@ -116,18 +116,25 @@ public class PublishComments implements Callable<VoidResult> {
     }
     drafts = drafts();
 
-    publishDrafts();
+    db.changes().beginTransaction(changeId);
+    try {
+      publishDrafts();
 
-    final boolean isCurrent = patchSetId.equals(change.currentPatchSetId());
+      final boolean isCurrent = patchSetId.equals(change.currentPatchSetId());
-    if (isCurrent && change.getStatus().isOpen()) {
+      if (isCurrent && change.getStatus().isOpen()) {
-      publishApprovals(ctl);
+        publishApprovals(ctl);
-    } else if (! approvals.isEmpty()) {
+      } else if (!approvals.isEmpty()) {
-      throw new InvalidChangeOperationException("Change is closed");
+        throw new InvalidChangeOperationException("Change is closed");
-    } else {
+      } else {
-      publishMessageOnly();
+        publishMessageOnly();
+      }
+
+      touchChange();
+      db.commit();
+    } finally {
+      db.rollback();
     }
 
-    touchChange();
     email();
     fireHook();
     return VoidResult.INSTANCE;