From 77c684b417cf101dafef86f9efbd20a287e764dd Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Fri, 28 Oct 2011 15:57:51 -0700 Subject: [PATCH] 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 --- .../rpc/patch/PatchDetailServiceImpl.java | 49 +++++++----- .../gerrit/httpd/rpc/patch/SaveDraft.java | 75 ++++++++++--------- .../gerrit/server/patch/PublishComments.java | 25 ++++--- 3 files changed, 88 insertions(+), 61 deletions(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java index 0f3b4f8eae..90a6253e54 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java @@ -110,18 +110,25 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements final AsyncCallback callback) { run(callback, new Action() { public VoidResult run(ReviewDb db) throws OrmException, Failure { - final PatchLineComment comment = db.patchComments().get(commentKey); - if (comment == null) { - throw new Failure(new NoSuchEntityException()); + Change.Id id = commentKey.getParentKey().getParentKey().getParentKey(); + db.changes().beginTransaction(id); + 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); - if (apr == null && reviewed) { - db.accountPatchReviews().insert( - Collections.singleton(new AccountPatchReview(patchKey, account))); - } else if (apr != null && !reviewed) { - db.accountPatchReviews().delete(Collections.singleton(apr)); + db.accounts().beginTransaction(account); + try { + AccountPatchReview apr = db.accountPatchReviews().get(key); + if (apr == null && reviewed) { + db.accountPatchReviews().insert( + 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; } }); } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/SaveDraft.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/SaveDraft.java index 1fbc559416..0f146353aa 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/SaveDraft.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/SaveDraft.java @@ -49,7 +49,6 @@ class SaveDraft extends Handler { this.changeControlFactory = changeControlFactory; this.db = db; this.currentUser = currentUser; - this.comment = comment; } @@ -62,42 +61,50 @@ class SaveDraft extends Handler { 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(); - 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)); - return nc; - - } else { - if (!me.equals(comment.getAuthor())) { + db.changes().beginTransaction(changeId); + try { + changeControlFactory.validateFor(changeId); + if (db.patchSets().get(patchSetId) == null) { throw new NoSuchChangeException(changeId); } - comment.updated(); - db.patchComments().update(Collections.singleton(comment)); - return comment; + + final Account.Id me = currentUser.getAccountId(); + 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(); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java index 838c31fdfe..a27312767a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java @@ -116,18 +116,25 @@ public class PublishComments implements Callable { } drafts = drafts(); - publishDrafts(); + db.changes().beginTransaction(changeId); + try { + publishDrafts(); - final boolean isCurrent = patchSetId.equals(change.currentPatchSetId()); - if (isCurrent && change.getStatus().isOpen()) { - publishApprovals(ctl); - } else if (! approvals.isEmpty()) { - throw new InvalidChangeOperationException("Change is closed"); - } else { - publishMessageOnly(); + final boolean isCurrent = patchSetId.equals(change.currentPatchSetId()); + if (isCurrent && change.getStatus().isOpen()) { + publishApprovals(ctl); + } else if (!approvals.isEmpty()) { + throw new InvalidChangeOperationException("Change is closed"); + } else { + publishMessageOnly(); + } + + touchChange(); + db.commit(); + } finally { + db.rollback(); } - touchChange(); email(); fireHook(); return VoidResult.INSTANCE;