From f26fdb499b26a2c02251546a7bb19703cc6c8bba Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Tue, 10 Jun 2014 21:47:04 +0200 Subject: [PATCH 1/5] ChangeMergeQueue: Fix race condition To the multiple triggers of submit action belong: * ChangeMergeQueue.merge() method called from the UI * ChangeMergeQueue.schedule() method called by background ReloadSubmitQueueOp active map in ChangeMergeQueue acts as a guard for submit job scheduling. However schedule() ignores the fact that the submit processing was already started for a specific branch and double submit action is scheduled. Specifically for cherry-pick submit strategy that cannot end good and fails with database constraints violation or similar, depending on the underlying database implementation. Guard the background scheduling similar to the UI scheduling. Defining unique index prevents the database corruption: [1], [2] but not the collision between manual and background merge jobs. [1] http://paste.openstack.org/show/83883 [2] http://paste.openstack.org/show/83888 Bug: issue 2034 Bug: issue 2383 Bug: issue 2702 Change-Id: I5b23f9d481351280e26412b82b525947338d9c00 --- .../java/com/google/gerrit/server/git/ChangeMergeQueue.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeMergeQueue.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeMergeQueue.java index a6b0a12e6e..bac7542b9d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeMergeQueue.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeMergeQueue.java @@ -148,9 +148,11 @@ public class ChangeMergeQueue implements MergeQueue { if (e == null) { e = new MergeEntry(branch); active.put(branch, e); + e.needMerge = true; + scheduleJob(e); + } else { + e.needMerge = true; } - e.needMerge = true; - scheduleJob(e); } @Override From 93d9d4a2e1c451c11cf040383ee57628cce50518 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 13 Jun 2014 15:02:19 +0900 Subject: [PATCH 2/5] Add missing 'link:' in 2.8.5 release notes Change-Id: I6268999ab31dd26b829b650d98d076f7223868bd --- ReleaseNotes/ReleaseNotes-2.8.5.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ReleaseNotes/ReleaseNotes-2.8.5.txt b/ReleaseNotes/ReleaseNotes-2.8.5.txt index 36f27d5924..4785642c19 100644 --- a/ReleaseNotes/ReleaseNotes-2.8.5.txt +++ b/ReleaseNotes/ReleaseNotes-2.8.5.txt @@ -73,7 +73,7 @@ ssh * Upgrade SSHD to version 0.11.0. + -Fixes https://code.google.com/p/gerrit/issues/detail?id=2406[Issue 2406]: +Fixes link:https://code.google.com/p/gerrit/issues/detail?id=2406[Issue 2406]: "git clone" hangs after 100% resolving deltas with git over SSH. + Fixes a number of other issues including a From 73eca0a738b5ba3c7e09d371c2712bc959fcf558 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 13 Jun 2014 14:59:28 +0900 Subject: [PATCH 3/5] Release notes for Gerrit 2.8.6 Change-Id: I72451095da8ee6973fbe140edbf9954420b2b795 --- ReleaseNotes/ReleaseNotes-2.8.6.txt | 23 +++++++++++++++++++++++ ReleaseNotes/index.txt | 1 + 2 files changed, 24 insertions(+) create mode 100644 ReleaseNotes/ReleaseNotes-2.8.6.txt diff --git a/ReleaseNotes/ReleaseNotes-2.8.6.txt b/ReleaseNotes/ReleaseNotes-2.8.6.txt new file mode 100644 index 0000000000..5bcade1222 --- /dev/null +++ b/ReleaseNotes/ReleaseNotes-2.8.6.txt @@ -0,0 +1,23 @@ +Release notes for Gerrit 2.8.6 +============================== + +There are no schema changes from link:ReleaseNotes-2.8.5.html[2.8.5]. + +Download: +link:https://gerrit-releases.storage.googleapis.com/gerrit-2.8.6.war[ +https://gerrit-releases.storage.googleapis.com/gerrit-2.8.6.war] + + +Bug Fixes +--------- + +* link:https://code.google.com/p/gerrit/issues/detail?id=2034[Issue 2034], +link:https://code.google.com/p/gerrit/issues/detail?id=2383[Issue 2383], +link:https://code.google.com/p/gerrit/issues/detail?id=2702[Issue 2702]: +Fix race condition in change merge queue. ++ +There was a race in the merge queue between changes submitted via +the UI, and merges scheduled by the background merge queue reload. ++ +This resulted in multiple submit actions being scheduled, leading +to corrupt changes. diff --git a/ReleaseNotes/index.txt b/ReleaseNotes/index.txt index ea988e69b9..368d15336d 100644 --- a/ReleaseNotes/index.txt +++ b/ReleaseNotes/index.txt @@ -4,6 +4,7 @@ Gerrit Code Review - Release Notes [[2_8]] Version 2.8.x ------------- +* link:ReleaseNotes-2.8.6.html[2.8.6] * link:ReleaseNotes-2.8.5.html[2.8.5] * link:ReleaseNotes-2.8.4.html[2.8.4] * link:ReleaseNotes-2.8.3.html[2.8.3] From c527b378f3c34ec75fd5b2051df19bec6d97de69 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 13 Jun 2014 15:09:27 +0900 Subject: [PATCH 4/5] Bump API version to 2.8.6 Change-Id: I358bca8e7187c50ddb2ca71498d130927fb1aeda --- VERSION | 2 +- gerrit-plugin-archetype/pom.xml | 2 +- gerrit-plugin-gwt-archetype/pom.xml | 2 +- gerrit-plugin-gwtui/pom.xml | 2 +- gerrit-plugin-js-archetype/pom.xml | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/VERSION b/VERSION index 9734d2dffa..ac95dd86e5 100644 --- a/VERSION +++ b/VERSION @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = '2.8.5' +GERRIT_VERSION = '2.8.6' diff --git a/gerrit-plugin-archetype/pom.xml b/gerrit-plugin-archetype/pom.xml index 4daf42ca8c..4eec8a7033 100644 --- a/gerrit-plugin-archetype/pom.xml +++ b/gerrit-plugin-archetype/pom.xml @@ -20,7 +20,7 @@ limitations under the License. com.google.gerrit gerrit-plugin-archetype - 2.8.5 + 2.8.6 Gerrit Code Review - Plugin Archetype diff --git a/gerrit-plugin-gwt-archetype/pom.xml b/gerrit-plugin-gwt-archetype/pom.xml index 11b59e6c2b..cc3c6e8f9c 100644 --- a/gerrit-plugin-gwt-archetype/pom.xml +++ b/gerrit-plugin-gwt-archetype/pom.xml @@ -20,7 +20,7 @@ limitations under the License. com.google.gerrit gerrit-plugin-gwt-archetype - 2.8.5 + 2.8.6 Gerrit Code Review - Web Ui GWT Plugin Archetype diff --git a/gerrit-plugin-gwtui/pom.xml b/gerrit-plugin-gwtui/pom.xml index 1128f69353..1b2bde1607 100644 --- a/gerrit-plugin-gwtui/pom.xml +++ b/gerrit-plugin-gwtui/pom.xml @@ -21,7 +21,7 @@ limitations under the License. com.google.gerrit gerrit-plugin-gwtui - 2.8.5 + 2.8.6 Gerrit Code Review - Plugin GWT UI diff --git a/gerrit-plugin-js-archetype/pom.xml b/gerrit-plugin-js-archetype/pom.xml index ffe3c6cde7..522c6d0fab 100644 --- a/gerrit-plugin-js-archetype/pom.xml +++ b/gerrit-plugin-js-archetype/pom.xml @@ -20,7 +20,7 @@ limitations under the License. com.google.gerrit gerrit-plugin-js-archetype - 2.8.5 + 2.8.6 Gerrit Code Review - Web UI JavaScript Plugin Archetype From d8090dc8a4d88c497b30320b913135296f6e6662 Mon Sep 17 00:00:00 2001 From: Bruce Zu Date: Fri, 13 Dec 2013 17:09:11 +0800 Subject: [PATCH 5/5] Executing CherryPick.writeCherryPickCommit() in a transaction. This would at least allow the partial changes to the database to rollback and not get users in a bad state when e.g. in the case of OrmDuplicateKeyException: patch_set_ancestors'. Bug: issue 2034 Bug: issue 2246 Change-Id: I65e111bedc3cec033299b5b5360aaee733d4b5c6 (cherry picked from commit eb4778d079e7156c82d04c2f26c71ca89d12df16) --- .../google/gerrit/server/git/CherryPick.java | 44 ++++++++++++------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/CherryPick.java index 48f62b5966..259d939f48 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/CherryPick.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/CherryPick.java @@ -161,26 +161,36 @@ public class CherryPick extends SubmitStrategy { ps.setCreatedOn(TimeUtil.nowTs()); ps.setUploader(submitAudit.getAccountId()); ps.setRevision(new RevId(newCommit.getId().getName())); - insertAncestors(args.db, ps.getId(), newCommit); - args.db.patchSets().insert(Collections.singleton(ps)); - n.change.setCurrentPatchSet(patchSetInfoFactory.get(newCommit, ps.getId())); - args.db.changes().update(Collections.singletonList(n.change)); + final RefUpdate ru; - final List approvals = Lists.newArrayList(); - for (PatchSetApproval a : args.mergeUtil.getApprovalsForCommit(n)) { - approvals.add(new PatchSetApproval(ps.getId(), a)); - } - args.db.patchSetApprovals().insert(approvals); + args.db.changes().beginTransaction(n.change.getId()); + try { + insertAncestors(args.db, ps.getId(), newCommit); + args.db.patchSets().insert(Collections.singleton(ps)); + n.change + .setCurrentPatchSet(patchSetInfoFactory.get(newCommit, ps.getId())); + args.db.changes().update(Collections.singletonList(n.change)); - final RefUpdate ru = args.repo.updateRef(ps.getRefName()); - ru.setExpectedOldObjectId(ObjectId.zeroId()); - ru.setNewObjectId(newCommit); - ru.disableRefLog(); - if (ru.update(args.rw) != RefUpdate.Result.NEW) { - throw new IOException(String.format("Failed to create ref %s in %s: %s", - ps.getRefName(), n.change.getDest().getParentKey().get(), - ru.getResult())); + final List approvals = Lists.newArrayList(); + for (PatchSetApproval a : args.mergeUtil.getApprovalsForCommit(n)) { + approvals.add(new PatchSetApproval(ps.getId(), a)); + } + args.db.patchSetApprovals().insert(approvals); + + ru = args.repo.updateRef(ps.getRefName()); + ru.setExpectedOldObjectId(ObjectId.zeroId()); + ru.setNewObjectId(newCommit); + ru.disableRefLog(); + if (ru.update(args.rw) != RefUpdate.Result.NEW) { + throw new IOException(String.format( + "Failed to create ref %s in %s: %s", ps.getRefName(), n.change + .getDest().getParentKey().get(), ru.getResult())); + } + + args.db.commit(); + } finally { + args.db.rollback(); } gitRefUpdated.fire(n.change.getProject(), ru);