diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java new file mode 100644 index 0000000000..3e490e86cb --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java @@ -0,0 +1,194 @@ +// 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.acceptance.server.notedb; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.TruthJUnit.assume; +import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB; + +import static java.util.stream.Collectors.toList; + +import com.google.common.collect.Iterables; +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.extensions.api.changes.DraftInput; +import com.google.gerrit.extensions.api.changes.ReviewInput; +import com.google.gerrit.extensions.client.ChangeStatus; +import com.google.gerrit.extensions.common.ApprovalInfo; +import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.common.CommentInfo; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.server.ReviewDbUtil; +import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.git.RepoRefCache; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.notedb.NoteDbChangeState; +import com.google.gerrit.testutil.NoteDbMode; +import com.google.inject.Inject; + +import org.eclipse.jgit.lib.Repository; +import org.junit.Before; +import org.junit.Test; + +import java.util.Collection; +import java.util.Collections; +import java.util.List; + +public class NoteDbPrimaryIT extends AbstractDaemonTest { + @Inject + private AllUsersName allUsers; + + @Before + public void setUp() throws Exception { + assume().that(NoteDbMode.get()).isEqualTo(NoteDbMode.READ_WRITE); + db = ReviewDbUtil.unwrapDb(db); + } + + @Test + public void updateChange() throws Exception { + PushOneCommit.Result r = createChange(); + Change.Id id = r.getChange().getId(); + setNoteDbPrimary(id); + + gApi.changes().id(id.get()).current().review(ReviewInput.approve()); + gApi.changes().id(id.get()).current().submit(); + + ChangeInfo info = gApi.changes().id(id.get()).get(); + assertThat(info.status).isEqualTo(ChangeStatus.MERGED); + ApprovalInfo approval = + Iterables.getOnlyElement(info.labels.get("Code-Review").all); + assertThat(approval._accountId).isEqualTo(admin.id.get()); + assertThat(approval.value).isEqualTo(2); + assertThat(info.messages).hasSize(3); + assertThat(Iterables.getLast(info.messages).message) + .isEqualTo("Change has been successfully merged by " + admin.fullName); + + ChangeNotes notes = notesFactory.create(db, project, id); + assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.MERGED); + assertThat(notes.getChange().getNoteDbState()) + .isEqualTo(NoteDbChangeState.NOTE_DB_PRIMARY_STATE); + + // Writes weren't reflected in ReviewDb. + assertThat(db.changes().get(id).getStatus()).isEqualTo(Change.Status.NEW); + assertThat(db.patchSetApprovals().byChange(id)).isEmpty(); + assertThat(db.changeMessages().byChange(id)).hasSize(1); + } + + @Test + public void deleteDraftComment() throws Exception { + PushOneCommit.Result r = createChange(); + Change.Id id = r.getChange().getId(); + setNoteDbPrimary(id); + + DraftInput din = new DraftInput(); + din.path = PushOneCommit.FILE_NAME; + din.line = 1; + din.message = "A comment"; + gApi.changes().id(id.get()).current().createDraft(din); + + CommentInfo di = Iterables.getOnlyElement( + gApi.changes().id(id.get()).current().drafts() + .get(PushOneCommit.FILE_NAME)); + assertThat(di.message).isEqualTo(din.message); + + assertThat( + db.patchComments().draftByChangeFileAuthor(id, din.path, admin.id)) + .isEmpty(); + + gApi.changes().id(id.get()).current().draft(di.id).delete(); + assertThat(gApi.changes().id(id.get()).current().drafts()).isEmpty(); + } + + @Test + public void deleteVote() throws Exception { + PushOneCommit.Result r = createChange(); + Change.Id id = r.getChange().getId(); + setNoteDbPrimary(id); + + gApi.changes().id(id.get()).current().review(ReviewInput.approve()); + List approvals = + gApi.changes().id(id.get()).get().labels.get("Code-Review").all; + assertThat(approvals).hasSize(1); + assertThat(approvals.get(0).value).isEqualTo(2); + + gApi.changes().id(id.get()).reviewer(admin.id.toString()) + .deleteVote("Code-Review"); + + approvals = gApi.changes().id(id.get()).get().labels.get("Code-Review").all; + assertThat(approvals).hasSize(1); + assertThat(approvals.get(0).value).isEqualTo(0); + } + + @Test + public void deleteVoteViaReview() throws Exception { + PushOneCommit.Result r = createChange(); + Change.Id id = r.getChange().getId(); + setNoteDbPrimary(id); + + gApi.changes().id(id.get()).current().review(ReviewInput.approve()); + List approvals = + gApi.changes().id(id.get()).get().labels.get("Code-Review").all; + assertThat(approvals).hasSize(1); + assertThat(approvals.get(0).value).isEqualTo(2); + + gApi.changes().id(id.get()).current().review(ReviewInput.noScore()); + + approvals = gApi.changes().id(id.get()).get().labels.get("Code-Review").all; + assertThat(approvals).hasSize(1); + assertThat(approvals.get(0).value).isEqualTo(0); + } + + @Test + public void deleteReviewer() throws Exception { + PushOneCommit.Result r = createChange(); + Change.Id id = r.getChange().getId(); + setNoteDbPrimary(id); + + gApi.changes().id(id.get()).addReviewer(user.id.toString()); + assertThat(getReviewers(id)).containsExactly(user.id); + gApi.changes().id(id.get()).reviewer(user.id.toString()).remove(); + assertThat(getReviewers(id)).isEmpty(); + } + + private void setNoteDbPrimary(Change.Id id) throws Exception { + Change c = db.changes().get(id); + assertThat(c).named("change " + id).isNotNull(); + NoteDbChangeState state = NoteDbChangeState.parse(c); + assertThat(state.getPrimaryStorage()) + .named("storage of " + id) + .isEqualTo(REVIEW_DB); + + try (Repository changeRepo = repoManager.openRepository(c.getProject()); + Repository allUsersRepo = repoManager.openRepository(allUsers)) { + assertThat( + state.isUpToDate( + new RepoRefCache(changeRepo), new RepoRefCache(allUsersRepo))) + .named("change " + id + " up to date") + .isTrue(); + } + + c.setNoteDbState(NoteDbChangeState.NOTE_DB_PRIMARY_STATE); + db.changes().update(Collections.singleton(c)); + } + + private List getReviewers(Change.Id id) throws Exception { + return gApi.changes().id(id.get()).get() + .reviewers.values().stream() + .flatMap(Collection::stream) + .map(a -> new Account.Id(a._accountId)) + .collect(toList()); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/CommentsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/CommentsUtil.java index d1e0ae5fb7..e65879b188 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/CommentsUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/CommentsUtil.java @@ -16,6 +16,7 @@ package com.google.gerrit.server; import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB; import static java.util.stream.Collectors.toList; import com.google.common.collect.ComparisonChain; @@ -42,6 +43,7 @@ import com.google.gerrit.server.git.BatchUpdate.ChangeContext; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; +import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.patch.PatchListNotAvailableException; @@ -357,8 +359,11 @@ public class CommentsUtil { for (Comment c : comments) { update.deleteComment(c); } - db.patchComments().delete(toPatchLineComments(update.getId(), - PatchLineComment.Status.DRAFT, comments)); + if (PrimaryStorage.of(update.getChange()) == REVIEW_DB) { + // Avoid OrmConcurrencyException trying to delete non-existent entities. + db.patchComments().delete(toPatchLineComments(update.getId(), + PatchLineComment.Status.DRAFT, comments)); + } } public void deleteAllDraftsFromAllUsers(Change.Id changeId) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java index 0dcf3bf928..3ca11d5d10 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java @@ -16,6 +16,7 @@ package com.google.gerrit.server; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB; import static com.google.gerrit.server.notedb.PatchSetState.DRAFT; import static com.google.gerrit.server.notedb.PatchSetState.PUBLISHED; @@ -27,6 +28,7 @@ import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; +import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.notedb.PatchSetState; import com.google.gwtorm.server.OrmException; @@ -126,7 +128,10 @@ public class PatchSetUtil { checkArgument(ps.isDraft(), "cannot delete non-draft patch set %s", ps.getId()); update.setPatchSetState(PatchSetState.DELETED); - db.patchSets().delete(Collections.singleton(ps)); + if (PrimaryStorage.of(update.getChange()) == REVIEW_DB) { + // Avoid OrmConcurrencyException trying to delete non-existent entities. + db.patchSets().delete(Collections.singleton(ps)); + } } private void ensurePatchSetMatches(PatchSet.Id psId, ChangeUpdate update) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java index d1f7cace9d..afec66ae58 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.change; import static com.google.common.base.Preconditions.checkState; +import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB; import com.google.gerrit.extensions.registration.DynamicItem; import com.google.gerrit.extensions.restapi.AuthException; @@ -32,6 +33,7 @@ import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.BatchUpdate.ChangeContext; import com.google.gerrit.server.git.BatchUpdate.RepoContext; import com.google.gerrit.server.git.BatchUpdateReviewDb; +import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -153,6 +155,10 @@ class DeleteChangeOp extends BatchUpdate.Op { private void deleteChangeElementsFromDb(ChangeContext ctx, Change.Id id) throws OrmException { + if (PrimaryStorage.of(ctx.getChange()) != REVIEW_DB) { + return; + } + // Avoid OrmConcurrencyException trying to delete non-existent entities. // Only delete from ReviewDb here; deletion from NoteDb is handled in // BatchUpdate. ReviewDb db = unwrap(ctx.getDb()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java index e473e39583..79cc35be4d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java @@ -14,6 +14,8 @@ package com.google.gerrit.server.change; +import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB; + import com.google.gerrit.common.TimeUtil; import com.google.gerrit.extensions.registration.DynamicItem; import com.google.gerrit.extensions.restapi.AuthException; @@ -34,6 +36,7 @@ import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.BatchUpdate.ChangeContext; import com.google.gerrit.server.git.BatchUpdate.RepoContext; import com.google.gerrit.server.git.UpdateException; +import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; import com.google.gerrit.server.project.NoSuchChangeException; @@ -146,11 +149,14 @@ public class DeleteDraftPatchSet implements RestModifyView approvals; + PrimaryStorage r = PrimaryStorage.of(ctx.getChange()); - if (migration.readChanges()) { + if (migration.readChanges() + && r == PrimaryStorage.REVIEW_DB) { // Because NoteDb and ReviewDb have different semantics for zero-value // approvals, we must fall back to ReviewDb as the source of truth here. ReviewDb db = ctx.getDb(); 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 aab40e027b..e1d1caa50f 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 @@ -15,6 +15,7 @@ package com.google.gerrit.server.change; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.data.LabelTypes; @@ -44,6 +45,7 @@ import com.google.gerrit.server.git.BatchUpdate.Context; import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.mail.send.DeleteVoteSender; import com.google.gerrit.server.mail.send.ReplyToChangeSender; +import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.util.LabelVote; import com.google.gwtorm.server.OrmException; @@ -166,8 +168,11 @@ public class DeleteVote } ctx.getUpdate(psId).removeApprovalFor(accountId, label); - ctx.getDb().patchSetApprovals().upsert( - Collections.singleton(deletedApproval(ctx))); + if (PrimaryStorage.of(ctx.getChange()) == REVIEW_DB) { + // Avoid OrmConcurrencyException trying to update non-existent entities. + ctx.getDb().patchSetApprovals().upsert( + Collections.singleton(deletedApproval(ctx))); + } StringBuilder msg = new StringBuilder(); msg.append("Removed "); 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 c930c82905..ce4476679d 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 @@ -17,6 +17,7 @@ package com.google.gerrit.server.change; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static com.google.gerrit.server.CommentsUtil.setCommentRevId; +import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB; import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.stream.Collectors.joining; @@ -83,6 +84,7 @@ import com.google.gerrit.server.git.BatchUpdate.Context; import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; +import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.project.ChangeControl; @@ -833,8 +835,11 @@ public class PostReview implements RestModifyView } forceCallerAsReviewer(ctx, current, ups, del); - ctx.getDb().patchSetApprovals().delete(del); - ctx.getDb().patchSetApprovals().upsert(ups); + if (PrimaryStorage.of(update.getChange()) == REVIEW_DB) { + // Avoid OrmConcurrencyException trying to delete non-existent entities. + ctx.getDb().patchSetApprovals().delete(del); + ctx.getDb().patchSetApprovals().upsert(ups); + } return !del.isEmpty() || !ups.isEmpty(); } 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 dc229b858b..3953f27517 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 @@ -53,7 +53,9 @@ import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.index.change.ChangeIndexer; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; +import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.notedb.NoteDbUpdateManager; +import com.google.gerrit.server.notedb.NoteDbUpdateManager.MismatchedStateException; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.InvalidChangeOperationException; @@ -61,7 +63,6 @@ import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.NoSuchRefException; import com.google.gerrit.server.util.RequestId; -import com.google.gwtorm.server.OrmConcurrencyException; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; @@ -937,10 +938,17 @@ public class BatchUpdate implements AutoCloseable { @SuppressWarnings("resource") // Not always opened. NoteDbUpdateManager updateManager = null; try { - ChangeContext ctx; + PrimaryStorage storage; db.changes().beginTransaction(id); try { - ctx = newChangeContext(db, repo, rw, id); + ChangeContext ctx = newChangeContext(db, repo, rw, id); + storage = PrimaryStorage.of(ctx.getChange()); + if (storage == PrimaryStorage.NOTE_DB + && !notesMigration.readChanges()) { + throw new OrmException( + "must have NoteDb enabled to update change " + id); + } + // Call updateChange on each op. logDebug("Calling updateChange on {} ops", changeOps.size()); for (Op op : changeOps) { @@ -960,34 +968,45 @@ public class BatchUpdate implements AutoCloseable { updateManager = stageNoteDbUpdate(ctx, deleted); } - // Bump lastUpdatedOn or rowVersion and commit. - Iterable cs = changesToUpdate(ctx); - if (newChanges.containsKey(id)) { - // Insert rather than upsert in case of a race on change IDs. - logDebug("Inserting change"); - db.changes().insert(cs); - } else if (deleted) { - logDebug("Deleting change"); - db.changes().delete(cs); + if (storage == PrimaryStorage.REVIEW_DB) { + // If primary storage of this change is in ReviewDb, bump + // lastUpdatedOn or rowVersion and commit. Otherwise, don't waste + // time updating ReviewDb at all. + Iterable cs = changesToUpdate(ctx); + if (isNewChange(id)) { + // Insert rather than upsert in case of a race on change IDs. + logDebug("Inserting change"); + db.changes().insert(cs); + } else if (deleted) { + logDebug("Deleting change"); + db.changes().delete(cs); + } else { + logDebug("Updating change"); + db.changes().update(cs); + } + if (!dryrun) { + db.commit(); + } } else { - logDebug("Updating change"); - db.changes().update(cs); - } - if (!dryrun) { - db.commit(); + logDebug( + "Skipping ReviewDb write since primary storage is {}", storage); } } finally { db.rollback(); } - if (notesMigration.commitChangeWrites()) { + // Do not execute the NoteDbUpdateManager, as we don't want too much + // contention on the underlying repo, and we would rather use a single + // ObjectInserter/BatchRefUpdate later. + // + // TODO(dborowitz): May or may not be worth trying to batch together + // flushed inserters as well. + if (storage == PrimaryStorage.NOTE_DB) { + // Should have failed above if NoteDb is disabled. + checkState(notesMigration.commitChangeWrites()); + noteDbResult = updateManager.stage().get(id); + } else if (notesMigration.commitChangeWrites()) { try { - // Do not execute the NoteDbUpdateManager, as we don't want too much - // contention on the underlying repo, and we would rather use a - // single ObjectInserter/BatchRefUpdate later. - // - // TODO(dborowitz): May or may not be worth trying to batch - // together flushed inserters as well. noteDbResult = updateManager.stage().get(id); } catch (IOException ex) { // Ignore all errors trying to update NoteDb at this point. We've @@ -1033,20 +1052,29 @@ public class BatchUpdate implements AutoCloseable { for (ChangeUpdate u : ctx.updates.values()) { updateManager.add(u); } + + Change c = ctx.getChange(); if (deleted) { - updateManager.deleteChange(ctx.getChange().getId()); + updateManager.deleteChange(c.getId()); } try { - updateManager.stageAndApplyDelta(ctx.getChange()); - } catch (OrmConcurrencyException ex) { - // Refused to apply update because NoteDb was out of sync. Go ahead with - // this ReviewDb update; it's still out of sync, but this is no worse - // than before, and it will eventually get rebuilt. - logDebug("Ignoring OrmConcurrencyException while staging"); + updateManager.stageAndApplyDelta(c); + } catch (MismatchedStateException ex) { + // Refused to apply update because NoteDb was out of sync, which can + // only happen if ReviewDb is the primary storage for this change. + // + // Go ahead with this ReviewDb update; it's still out of sync, but this + // is no worse than before, and it will eventually get rebuilt. + logDebug("Ignoring MismatchedStateException while staging"); } + return updateManager; } + private boolean isNewChange(Change.Id id) { + return newChanges.containsKey(id); + } + private void logDebug(String msg, Throwable t) { if (log.isDebugEnabled()) { BatchUpdate.this.logDebug("[" + taskId + "]" + msg, t); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java index 3c669f09de..c00035c8ad 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java @@ -27,6 +27,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk; +import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -109,16 +110,20 @@ public abstract class AbstractChangeNotes { } protected final Args args; + protected final PrimaryStorage primaryStorage; protected final boolean autoRebuild; private final Change.Id changeId; private ObjectId revision; private boolean loaded; - AbstractChangeNotes(Args args, Change.Id changeId, boolean autoRebuild) { + AbstractChangeNotes(Args args, Change.Id changeId, + @Nullable PrimaryStorage primaryStorage, boolean autoRebuild) { this.args = checkNotNull(args); this.changeId = checkNotNull(changeId); - this.autoRebuild = autoRebuild; + this.primaryStorage = primaryStorage; + this.autoRebuild = primaryStorage == PrimaryStorage.REVIEW_DB + && autoRebuild; } public Change.Id getChangeId() { @@ -135,6 +140,9 @@ public abstract class AbstractChangeNotes { return self(); } boolean read = args.migration.readChanges(); + if (!read && primaryStorage == PrimaryStorage.NOTE_DB) { + throw new OrmException("NoteDb is required to read change " + changeId); + } boolean readOrWrite = read || args.migration.writeChanges(); if (!readOrWrite && !autoRebuild) { loadDefaults(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java index eda50d726c..d0b0c5484b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java @@ -98,16 +98,7 @@ public class ChangeNotes extends AbstractChangeNotes { public static Change readOneReviewDbChange(ReviewDb db, Change.Id id) throws OrmException { - return checkNoteDbState(ReviewDbUtil.unwrapDb(db).changes().get(id)); - } - - private static Change checkNoteDbState(Change c) throws OrmException { - NoteDbChangeState s = NoteDbChangeState.parse(c); - if (s != null && s.getPrimaryStorage() != PrimaryStorage.REVIEW_DB) { - throw new OrmException( - "invalid NoteDbChangeState in " + c.getId() + ": " + s); - } - return c; + return ReviewDbUtil.unwrapDb(db).changes().get(id); } @Singleton @@ -276,7 +267,6 @@ public class ChangeNotes extends AbstractChangeNotes { } } else { for (Change change : ReviewDbUtil.unwrapDb(db).changes().all()) { - checkNoteDbState(change); ChangeNotes notes = createFromChangeOnlyWhenNoteDbDisabled(change); if (predicate.test(notes)) { m.put(change.getProject(), notes); @@ -373,7 +363,7 @@ public class ChangeNotes extends AbstractChangeNotes { private ChangeNotes(Args args, Change change, boolean autoRebuild, @Nullable RefCache refs) { - super(args, change.getId(), autoRebuild); + super(args, change.getId(), PrimaryStorage.of(change), autoRebuild); this.change = new Change(change); this.refs = refs; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesState.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesState.java index e0f7920f77..d5abdee929 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesState.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesState.java @@ -31,6 +31,7 @@ import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.Comment; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.ReviewerStatusUpdate; @@ -141,7 +142,10 @@ public abstract class ChangeNotesState { abstract Timestamp createdOn(); abstract Timestamp lastUpdatedOn(); abstract Account.Id owner(); - abstract String branch(); // Project not included. + + // Project not included, as it's not stored anywhere in the meta ref. + abstract String branch(); + @Nullable abstract PatchSet.Id currentPatchSetId(); abstract String subject(); @Nullable abstract String topic(); @@ -172,17 +176,35 @@ public abstract class ChangeNotesState { changeMessagesByPatchSet(); abstract ImmutableListMultimap publishedComments(); + Change newChange(Project.NameKey project) { + ChangeColumns c = checkNotNull(columns(), "columns are required"); + Change change = new Change( + c.changeKey(), + changeId(), + c.owner(), + new Branch.NameKey(project, c.branch()), + c.createdOn()); + copyNonConstructorColumnsTo(change); + change.setNoteDbState(NoteDbChangeState.NOTE_DB_PRIMARY_STATE); + return change; + } + void copyColumnsTo(Change change) { - ChangeColumns c = checkNotNull(columns()); + ChangeColumns c = checkNotNull(columns(), "columns are required"); + change.setKey(c.changeKey()); + change.setOwner(c.owner()); + change.setDest(new Branch.NameKey(change.getProject(), c.branch())); + change.setCreatedOn(c.createdOn()); + copyNonConstructorColumnsTo(change); + } + + private void copyNonConstructorColumnsTo(Change change) { + ChangeColumns c = checkNotNull(columns(), "columns are required"); if (c.status() != null) { change.setStatus(c.status()); } - change.setKey(c.changeKey()); - change.setDest(new Branch.NameKey(change.getProject(), c.branch())); change.setTopic(Strings.emptyToNull(c.topic())); - change.setCreatedOn(c.createdOn()); change.setLastUpdatedOn(c.lastUpdatedOn()); - change.setOwner(c.owner()); change.setSubmissionId(c.submissionId()); change.setAssignee(c.assignee()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java index 661112e26d..7ba90a21a5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java @@ -31,6 +31,7 @@ import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.git.RepoRefCache; +import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.notedb.NoteDbUpdateManager.StagedResult; import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder; import com.google.gerrit.server.project.NoSuchChangeException; @@ -85,7 +86,9 @@ public class DraftCommentNotes extends AbstractChangeNotes { Args args, @Assisted Change.Id changeId, @Assisted Account.Id author) { - super(args, changeId, true); + // PrimaryStorage is unknown; this should only called by + // PatchLineCommentsUtil#draftByAuthor, which can live with this. + super(args, changeId, null, false); this.change = null; this.author = author; this.rebuildResult = null; @@ -97,7 +100,7 @@ public class DraftCommentNotes extends AbstractChangeNotes { Account.Id author, boolean autoRebuild, NoteDbUpdateManager.Result rebuildResult) { - super(args, change.getId(), autoRebuild); + super(args, change.getId(), PrimaryStorage.of(change), autoRebuild); this.change = change; this.author = author; this.rebuildResult = rebuildResult; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbChangeState.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbChangeState.java index 2fefd72879..80f9eaf31b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbChangeState.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbChangeState.java @@ -34,12 +34,14 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.server.ReviewDbUtil; import com.google.gerrit.server.git.RefCache; + +import org.eclipse.jgit.lib.ObjectId; + import java.io.IOException; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; -import org.eclipse.jgit.lib.ObjectId; /** * The state of all relevant NoteDb refs across all repos corresponding to a @@ -61,19 +63,21 @@ public class NoteDbChangeState { public static final String NOTE_DB_PRIMARY_STATE = "N"; public enum PrimaryStorage { - REVIEW_DB('R', true), - NOTE_DB('N', false); + REVIEW_DB('R'), + NOTE_DB('N'); private final char code; - private final boolean writeToReviewDb; - private PrimaryStorage(char code, boolean writeToReviewDb) { + private PrimaryStorage(char code) { this.code = code; - this.writeToReviewDb = writeToReviewDb; } - public boolean writeToReviewDb() { - return writeToReviewDb; + public static PrimaryStorage of(Change c) { + return of(NoteDbChangeState.parse(c)); + } + + public static PrimaryStorage of(NoteDbChangeState s) { + return s != null ? s.getPrimaryStorage() : REVIEW_DB; } } @@ -232,6 +236,9 @@ public class NoteDbChangeState { public static boolean isChangeUpToDate(@Nullable NoteDbChangeState state, RefCache changeRepoRefs, Change.Id changeId) throws IOException { + if (PrimaryStorage.of(state) == NOTE_DB) { + return true; // Primary storage is NoteDb, up to date by definition. + } if (state == null) { return !changeRepoRefs.get(changeMetaRef(changeId)).isPresent(); } @@ -241,6 +248,9 @@ public class NoteDbChangeState { public static boolean areDraftsUpToDate(@Nullable NoteDbChangeState state, RefCache draftsRepoRefs, Change.Id changeId, Account.Id accountId) throws IOException { + if (PrimaryStorage.of(state) == NOTE_DB) { + return true; // Primary storage is NoteDb, up to date by definition. + } if (state == null) { return !draftsRepoRefs.get(refsDraftComments(changeId, accountId)) .isPresent(); @@ -284,6 +294,9 @@ public class NoteDbChangeState { } public boolean isChangeUpToDate(RefCache changeRepoRefs) throws IOException { + if (primaryStorage == NOTE_DB) { + return true; // Primary storage is NoteDb, up to date by definition. + } Optional id = changeRepoRefs.get(changeMetaRef(changeId)); if (!id.isPresent()) { return getChangeMetaId().equals(ObjectId.zeroId()); @@ -293,6 +306,9 @@ public class NoteDbChangeState { public boolean areDraftsUpToDate(RefCache draftsRepoRefs, Account.Id accountId) throws IOException { + if (primaryStorage == NOTE_DB) { + return true; // Primary storage is NoteDb, up to date by definition. + } Optional id = draftsRepoRefs.get(refsDraftComments(changeId, accountId)); if (!id.isPresent()) { @@ -303,6 +319,9 @@ public class NoteDbChangeState { public boolean isUpToDate(RefCache changeRepoRefs, RefCache draftsRepoRefs) throws IOException { + if (primaryStorage == NOTE_DB) { + return true; // Primary storage is NoteDb, up to date by definition. + } if (!isChangeUpToDate(changeRepoRefs)) { return false; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java index 1f64bcc359..1e5cb1ea7d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java @@ -37,6 +37,7 @@ import com.google.gerrit.server.git.ChainedReceiveCommands; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.InMemoryInserter; import com.google.gerrit.server.git.InsertedObject; +import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gwtorm.server.OrmConcurrencyException; import com.google.gwtorm.server.OrmException; import com.google.inject.assistedinject.Assisted; @@ -489,6 +490,17 @@ public class NoteDbUpdateManager implements AutoCloseable { } } + public static class MismatchedStateException extends OrmException { + private static final long serialVersionUID = 1L; + + private MismatchedStateException(Change.Id id, NoteDbChangeState expectedState) { + super(String.format( + "cannot apply NoteDb updates for change %s;" + + " change meta ref does not match %s", + id, expectedState.getChangeMetaId().name())); + } + } + private void checkExpectedState() throws OrmException, IOException { if (!checkExpectedState) { return; @@ -513,15 +525,17 @@ public class NoteDbUpdateManager implements AutoCloseable { // - We short-circuited before adding any commands that update this // ref, and we won't stage a delta for this change either. // Either way, it is safe to proceed here rather than throwing - // OrmConcurrencyException. + // MismatchedStateException. + continue; + } + + if (expectedState.getPrimaryStorage() == PrimaryStorage.NOTE_DB) { + // NoteDb is primary, no need to compare state to ReviewDb. continue; } if (!expectedState.isChangeUpToDate(changeRepo.cmds.getRepoRefCache())) { - throw new OrmConcurrencyException(String.format( - "cannot apply NoteDb updates for change %s;" - + " change meta ref does not match %s", - u.getId(), expectedState.getChangeMetaId().name())); + throw new MismatchedStateException(u.getId(), expectedState); } } @@ -529,7 +543,8 @@ public class NoteDbUpdateManager implements AutoCloseable { ChangeDraftUpdate u = us.iterator().next(); NoteDbChangeState expectedState = NoteDbChangeState.parse(u.getChange()); - if (expectedState == null) { + if (expectedState == null + || expectedState.getPrimaryStorage() == PrimaryStorage.NOTE_DB) { continue; // See above. } @@ -539,7 +554,8 @@ public class NoteDbUpdateManager implements AutoCloseable { throw new OrmConcurrencyException(String.format( "cannot apply NoteDb updates for change %s;" + " draft ref for account %s does not match %s", - u.getId(), accountId, expectedState.getChangeMetaId().name())); + u.getId(), accountId, + expectedState.getChangeMetaId().name())); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RobotCommentNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RobotCommentNotes.java index 4a26d7efae..f60fd2d2b2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RobotCommentNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RobotCommentNotes.java @@ -22,6 +22,7 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.client.RobotComment; +import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.AssistedInject; @@ -47,7 +48,7 @@ public class RobotCommentNotes extends AbstractChangeNotes { RobotCommentNotes( Args args, @Assisted Change change) { - super(args, change.getId(), false); + super(args, change.getId(), PrimaryStorage.of(change), false); this.change = change; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java index b3aa420afa..20524cf568 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java @@ -57,6 +57,7 @@ import com.google.gerrit.server.notedb.ChangeNoteUtil; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.notedb.NoteDbChangeState; +import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.notedb.NoteDbUpdateManager; import com.google.gerrit.server.notedb.NoteDbUpdateManager.OpenRepo; import com.google.gerrit.server.notedb.NoteDbUpdateManager.Result; @@ -185,7 +186,8 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { public NoteDbUpdateManager stage(ReviewDb db, Change.Id changeId) throws NoSuchChangeException, IOException, OrmException { db = ReviewDbUtil.unwrapDb(db); - Change change = ChangeNotes.readOneReviewDbChange(db, changeId); + Change change = + checkNoteDbState(ChangeNotes.readOneReviewDbChange(db, changeId)); if (change == null) { throw new NoSuchChangeException(changeId); } @@ -201,7 +203,8 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { NoteDbUpdateManager manager) throws NoSuchChangeException, OrmException, IOException { db = ReviewDbUtil.unwrapDb(db); - Change change = ChangeNotes.readOneReviewDbChange(db, changeId); + Change change = + checkNoteDbState(ChangeNotes.readOneReviewDbChange(db, changeId)); if (change == null) { throw new NoSuchChangeException(changeId); } @@ -254,6 +257,16 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { return r; } + private static Change checkNoteDbState(Change c) throws OrmException { + // Can only rebuild a change if its primary storage is ReviewDb. + NoteDbChangeState s = NoteDbChangeState.parse(c); + if (s != null && s.getPrimaryStorage() != PrimaryStorage.REVIEW_DB) { + throw new OrmException(String.format( + "cannot rebuild change " + c.getId() + " with state " + s)); + } + return c; + } + @Override public void buildUpdates(NoteDbUpdateManager manager, ChangeBundle bundle) throws IOException, OrmException {