BatchUpdate: Key ChangeUpdates by patch set Allow callers to create one update per patch set of the change, looking up by patch set ID and creating as necessary. BatchUpdate will take care of creating a BatchMetaDataUpdate and applying the updates in order, oldest patch set to newest patch set. Force callers to specify the patch set instead of implicitly using the current patch set. There were some places we were forgetting to do this, so it's good to have made it required. We will eventually need to update multiple patch sets at once during submit, where approvals may be copied between patch sets. Change-Id: I35e9378d6f9b494db516f8d8c38c5b6e75c2f4c7
diff --git a/gerrit-reviewdb/BUCK b/gerrit-reviewdb/BUCK index 7bed0f3..82e0135 100644 --- a/gerrit-reviewdb/BUCK +++ b/gerrit-reviewdb/BUCK
@@ -19,6 +19,7 @@ resources = glob(['src/main/resources/**/*']), deps = [ '//gerrit-extension-api:api', + '//lib:guava', '//lib:gwtorm', ], visibility = ['PUBLIC'],
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java new file mode 100644 index 0000000..7cec109 --- /dev/null +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java
@@ -0,0 +1,41 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.reviewdb.server; + +import com.google.common.base.Function; +import com.google.common.collect.Ordering; +import com.google.gwtorm.client.IntKey; + +/** Static utilities for ReviewDb types. */ +public class ReviewDbUtil { + private static final Function<IntKey<?>, Integer> INT_KEY_FUNCTION = + new Function<IntKey<?>, Integer>() { + @Override + public Integer apply(IntKey<?> in) { + return in.get(); + } + }; + + private static final Ordering<? extends IntKey<?>> INT_KEY_ORDERING = + Ordering.natural().onResultOf(INT_KEY_FUNCTION); + + @SuppressWarnings("unchecked") + public static <K extends IntKey<?>> Ordering<K> intKeyOrdering() { + return (Ordering<K>) INT_KEY_ORDERING; + } + + private ReviewDbUtil() { + } +}
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 2c934d6..85516cf7 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
@@ -119,14 +119,15 @@ public void updateChange(ChangeContext ctx) throws OrmException, ResourceConflictException { change = ctx.getChange(); - ChangeUpdate update = ctx.getUpdate(); + PatchSet.Id psId = change.currentPatchSetId(); + ChangeUpdate update = ctx.getUpdate(psId); if (change == null || !change.getStatus().isOpen()) { throw new ResourceConflictException("change is " + status(change)); } else if (change.getStatus() == Change.Status.DRAFT) { throw new ResourceConflictException( "draft changes cannot be abandoned"); } - patchSet = ctx.getDb().patchSets().get(change.currentPatchSetId()); + patchSet = ctx.getDb().patchSets().get(psId); change.setStatus(Change.Status.ABANDONED); change.setLastUpdatedOn(ctx.getWhen()); ctx.getDb().changes().update(Collections.singleton(change));
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java index 5e7d1c5..1535422 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -242,10 +242,10 @@ change = ctx.getChange(); // Use defensive copy created by ChangeControl. ReviewDb db = ctx.getDb(); ChangeControl ctl = ctx.getControl(); - ChangeUpdate update = ctx.getUpdate(); patchSetInfo = patchSetInfoFactory.get( ctx.getRevWalk(), commit, patchSet.getId()); ctx.getChange().setCurrentPatchSet(patchSetInfo); + ChangeUpdate update = ctx.getUpdate(patchSet.getId()); if (patchSet.getGroups() == null) { patchSet.setGroups(GroupCollector.getDefaultGroups(patchSet));
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java index fe1bd59..2143604 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java
@@ -121,7 +121,7 @@ setCommentRevId( comment, patchListCache, ctx.getChange(), ps); plcUtil.insertComments( - ctx.getDb(), ctx.getUpdate(), Collections.singleton(comment)); + ctx.getDb(), ctx.getUpdate(psId), Collections.singleton(comment)); } } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftComment.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftComment.java index 94e6b34..ac3ecd2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftComment.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftComment.java
@@ -97,7 +97,7 @@ PatchLineComment c = maybeComment.get(); setCommentRevId(c, patchListCache, ctx.getChange(), ps); plcUtil.deleteComments( - ctx.getDb(), ctx.getUpdate(), Collections.singleton(c)); + ctx.getDb(), ctx.getUpdate(psId), Collections.singleton(c)); } } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java index c8ed0f5..ea34f1d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java
@@ -111,8 +111,7 @@ .append("\n"); psa = a; a.setValue((short)0); - ctx.getUpdate().setPatchSetId(psId); - ctx.getUpdate().removeApprovalFor(a.getAccountId(), label); + ctx.getUpdate(psId).removeApprovalFor(a.getAccountId(), label); break; } } else { @@ -133,7 +132,7 @@ ctx.getWhen(), change.currentPatchSetId()); changeMessage.setMessage(msg.toString()); - cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(), + cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(psId), changeMessage); } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java index daca350..1810dbd 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -209,7 +209,7 @@ ChangeControl ctl = ctx.getControl(); change = ctx.getChange(); - ChangeUpdate update = ctx.getUpdate(); + ChangeUpdate update = ctx.getUpdate(psId); if (!change.getStatus().isOpen() && !allowClosed) { throw new InvalidChangeOperationException(String.format( @@ -222,8 +222,6 @@ patchSet.setRevision(new RevId(commit.name())); patchSet.setDraft(draft); - update.setPatchSetId(patchSet.getId()); - if (groups != null) { patchSet.setGroups(groups); } else { @@ -251,7 +249,7 @@ db.changes().update(Collections.singleton(change)); approvalCopier.copy(db, ctl, patchSet); if (changeMessage != null) { - cmUtil.addChangeMessage(db, ctx.getUpdate(), changeMessage); + cmUtil.addChangeMessage(db, update, changeMessage); } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index 1730e2f..99e27d6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
@@ -358,7 +358,6 @@ change.setLastUpdatedOn(ctx.getWhen()); } ps = ctx.getDb().patchSets().get(psId); - ctx.getUpdate().setPatchSetId(psId); boolean dirty = false; dirty |= insertComments(ctx); dirty |= updateLabels(ctx); @@ -466,8 +465,11 @@ } break; } - plcUtil.deleteComments(ctx.getDb(), ctx.getUpdate(), del); - plcUtil.upsertComments(ctx.getDb(), ctx.getUpdate(), ups); + ChangeUpdate u = ctx.getUpdate(psId); + // TODO(dborowitz): Currently doesn't work for PUBLISH_ALL_REVISIONS with + // notedb. + plcUtil.deleteComments(ctx.getDb(), u, del); + plcUtil.upsertComments(ctx.getDb(), u, ups); comments.addAll(ups); return !del.isEmpty() || !ups.isEmpty(); } @@ -513,7 +515,7 @@ List<PatchSetApproval> ups = Lists.newArrayList(); Map<String, PatchSetApproval> current = scanLabels(ctx, del); - ChangeUpdate update = ctx.getUpdate(); + ChangeUpdate update = ctx.getUpdate(psId); LabelTypes labelTypes = ctx.getControl().getLabelTypes(); for (Map.Entry<String, Short> ent : labels.entrySet()) { String name = ent.getKey(); @@ -644,7 +646,7 @@ "Patch Set %d:%s", psId.get(), buf.toString())); - cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(), message); + cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(psId), message); return true; }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishDraftPatchSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishDraftPatchSet.java index 5f5c4cd..68a05ee 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishDraftPatchSet.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishDraftPatchSet.java
@@ -182,7 +182,7 @@ private void saveChange(ChangeContext ctx) throws OrmException { change = ctx.getChange(); - ChangeUpdate update = ctx.getUpdate(); + ChangeUpdate update = ctx.getUpdate(psId); wasDraftChange = change.getStatus() == Change.Status.DRAFT; if (wasDraftChange) { change.setStatus(Change.Status.NEW); @@ -219,7 +219,7 @@ recipients = getRecipientsFromFooters(accountResolver, patchSet, footerLines); recipients.remove(ctx.getUser().getAccountId()); - approvalsUtil.addReviewers(ctx.getDb(), ctx.getUpdate(), labelTypes, + approvalsUtil.addReviewers(ctx.getDb(), ctx.getUpdate(psId), labelTypes, change, patchSet, patchSetInfo, recipients.getReviewers(), oldReviewers); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java index 33682ad..39a85ba 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java
@@ -35,6 +35,7 @@ import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.BatchUpdate.ChangeContext; import com.google.gerrit.server.git.UpdateException; +import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.patch.PatchListCache; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -116,6 +117,7 @@ comment = maybeComment.get(); PatchSet.Id psId = comment.getKey().getParentKey().getParentKey(); + ChangeUpdate update = ctx.getUpdate(psId); PatchSet ps = ctx.getDb().patchSets().get(psId); if (ps == null) { throw new ResourceNotFoundException("patch set not found: " + psId); @@ -126,7 +128,7 @@ // Delete then recreate the comment instead of an update. plcUtil.deleteComments( - ctx.getDb(), ctx.getUpdate(), Collections.singleton(comment)); + ctx.getDb(), update, Collections.singleton(comment)); comment = new PatchLineComment( new PatchLineComment.Key( new Patch.Key(psId, in.path), @@ -135,14 +137,14 @@ ctx.getUser().getAccountId(), comment.getParentUuid(), ctx.getWhen()); setCommentRevId(comment, patchListCache, ctx.getChange(), ps); - plcUtil.insertComments(ctx.getDb(), ctx.getUpdate(), + plcUtil.insertComments(ctx.getDb(), update, Collections.singleton(update(comment, in))); } else { if (comment.getRevId() == null) { setCommentRevId( comment, patchListCache, ctx.getChange(), ps); } - plcUtil.updateComments(ctx.getDb(), ctx.getUpdate(), + plcUtil.updateComments(ctx.getDb(), update, Collections.singleton(update(comment, in))); } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutTopic.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutTopic.java index a8553a9..6590c55 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutTopic.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutTopic.java
@@ -34,6 +34,7 @@ import com.google.gerrit.server.git.BatchUpdate.ChangeContext; import com.google.gerrit.server.git.BatchUpdate.Context; import com.google.gerrit.server.git.UpdateException; +import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.project.ChangeControl; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -101,6 +102,7 @@ @Override public void updateChange(ChangeContext ctx) throws OrmException { change = ctx.getChange(); + ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId()); newTopicName = Strings.nullToEmpty(input.topic); oldTopicName = Strings.nullToEmpty(change.getTopic()); if (oldTopicName.equals(newTopicName)) { @@ -116,7 +118,7 @@ oldTopicName, newTopicName); } change.setTopic(Strings.emptyToNull(newTopicName)); - ctx.getUpdate().setTopic(change.getTopic()); + update.setTopic(change.getTopic()); ChangeUtil.updated(change); ctx.getDb().changes().update(Collections.singleton(change)); @@ -127,7 +129,7 @@ caller.getAccountId(), ctx.getWhen(), change.currentPatchSetId()); cmsg.setMessage(summary); - cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(), cmsg); + cmUtil.addChangeMessage(ctx.getDb(), update, cmsg); } @Override
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 bc3301a..0da733c 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
@@ -110,11 +110,12 @@ ResourceConflictException { caller = ctx.getUser().asIdentifiedUser(); change = ctx.getChange(); - ChangeUpdate update = ctx.getUpdate(); + PatchSet.Id psId = change.currentPatchSetId(); + ChangeUpdate update = ctx.getUpdate(psId); if (change == null || change.getStatus() != Status.ABANDONED) { throw new ResourceConflictException("change is " + status(change)); } - patchSet = ctx.getDb().patchSets().get(change.currentPatchSetId()); + patchSet = ctx.getDb().patchSets().get(psId); change.setStatus(Status.NEW); change.setLastUpdatedOn(ctx.getWhen()); ctx.getDb().changes().update(Collections.singleton(change));
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/SetHashtagsOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/SetHashtagsOp.java index c590e03..48a17b4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/SetHashtagsOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/SetHashtagsOp.java
@@ -83,7 +83,8 @@ if (!ctx.getControl().canEditHashtags()) { throw new AuthException("Editing hashtags not permitted"); } - ChangeUpdate update = ctx.getUpdate(); + change = ctx.getChange(); + ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId()); ChangeNotes notes = update.getChangeNotes().load(); Set<String> existingHashtags = notes.getHashtags(); @@ -110,7 +111,6 @@ update.setHashtags(updated); } - change = update.getChange(); updatedHashtags = ImmutableSortedSet.copyOf(updated); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java index 77026b1..8755ffa 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java
@@ -25,11 +25,14 @@ import com.google.common.util.concurrent.CheckedFuture; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Change; +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.reviewdb.server.ReviewDbUtil; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; +import com.google.gerrit.server.git.VersionedMetaData.BatchMetaDataUpdate; import com.google.gerrit.server.index.ChangeIndexer; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; @@ -54,6 +57,7 @@ import java.util.List; import java.util.Map; import java.util.TimeZone; +import java.util.TreeMap; /** * Context for a set of updates that should be applied for a site. @@ -159,20 +163,27 @@ public class ChangeContext extends Context { private final ChangeControl ctl; - private final ChangeUpdate update; + private final Map<PatchSet.Id, ChangeUpdate> updates; + private boolean deleted; private ChangeContext(ChangeControl ctl) { this.ctl = ctl; - this.update = changeUpdateFactory.create(ctl, when); + updates = new TreeMap<>(ReviewDbUtil.intKeyOrdering()); } - public ChangeUpdate getUpdate() { - return update; + public ChangeUpdate getUpdate(PatchSet.Id psId) { + ChangeUpdate u = updates.get(psId); + if (u == null) { + u = changeUpdateFactory.create(ctl, when); + u.setPatchSetId(psId); + updates.put(psId, u); + } + return u; } public ChangeNotes getNotes() { - return update.getChangeNotes(); + return ctl.getNotes(); } public ChangeControl getControl() { @@ -180,7 +191,7 @@ } public Change getChange() { - return update.getChange(); + return ctl.getChange(); } public void markDeleted() { @@ -523,7 +534,18 @@ } finally { db.rollback(); } - ctx.getUpdate().commit(); + + BatchMetaDataUpdate bmdu = null; + for (ChangeUpdate u : ctx.updates.values()) { + if (bmdu == null) { + bmdu = u.openUpdate(); + } + u.writeCommit(bmdu); + } + if (bmdu != null) { + bmdu.commit(); + } + if (ctx.deleted) { indexFutures.add(indexer.deleteAsync(id)); } else {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index c628c45..8defdb0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -1807,7 +1807,7 @@ new BatchUpdate.Op() { @Override public void updateChange(ChangeContext ctx) throws Exception { - ctx.getUpdate().setTopic(magicBranch.topic); + ctx.getUpdate(ps.getId()).setTopic(magicBranch.topic); } }); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java index e27bd5f..7926eac 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
@@ -167,7 +167,6 @@ // Merge conflict; don't update change. return; } - ctx.getUpdate().setPatchSetId(psId); PatchSet ps = new PatchSet(psId); ps.setCreatedOn(ctx.getWhen()); ps.setUploader(args.caller.getAccountId()); @@ -183,7 +182,7 @@ for (PatchSetApproval a : args.approvalsUtil.byPatchSet( args.db, toMerge.getControl(), toMerge.getPatchsetId())) { approvals.add(new PatchSetApproval(ps.getId(), a)); - ctx.getUpdate().putApproval(a.getLabel(), a.getValue()); + ctx.getUpdate(psId).putApproval(a.getLabel(), a.getValue()); } args.db.patchSetApprovals().insert(approvals);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java index f896d89..d948a15 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
@@ -386,9 +386,6 @@ BatchMetaDataUpdate batch = openUpdate(); try { writeCommit(batch); - if (draftUpdate != null) { - draftUpdate.commit(); - } RevCommit c = batch.commit(); return c; } catch (OrmException e) { @@ -409,6 +406,9 @@ } } batch.write(this, builder); + if (draftUpdate != null) { + draftUpdate.commit(); + } } @Override