From ef3fb119481d590d1719374b98525f25fa3fca0d Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 4 Nov 2016 17:37:39 -0400 Subject: [PATCH 1/4] ChangeRebuilder: Add method to rebuild NoteDb -> ReviewDb Change-Id: Iad57de65f54b5ac176576748972fe404c92f6e85 --- .../server/notedb/NoteDbPrimaryIT.java | 74 ++++++++++++++++++- .../gerrit/server/notedb/NoteDbModule.java | 6 ++ .../notedb/TestChangeRebuilderWrapper.java | 11 +++ .../notedb/rebuild/ChangeRebuilder.java | 12 +++ .../notedb/rebuild/ChangeRebuilderImpl.java | 52 +++++++++++++ 5 files changed, 154 insertions(+), 1 deletion(-) 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 index ef5e7cf3d0..542029ae26 100644 --- 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 @@ -27,6 +27,8 @@ import com.github.rholder.retry.Retryer; import com.github.rholder.retry.RetryerBuilder; import com.github.rholder.retry.StopStrategies; import com.google.common.base.Throwables; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.PushOneCommit; @@ -34,6 +36,7 @@ import com.google.gerrit.common.Nullable; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.extensions.api.changes.DraftInput; import com.google.gerrit.extensions.api.changes.ReviewInput; +import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput; import com.google.gerrit.extensions.client.ChangeStatus; import com.google.gerrit.extensions.common.ApprovalInfo; import com.google.gerrit.extensions.common.ChangeInfo; @@ -41,9 +44,14 @@ import com.google.gerrit.extensions.common.CommentInfo; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.server.ReviewDbUtil; +import com.google.gerrit.server.ChangeMessagesUtil; +import com.google.gerrit.server.CommentsUtil; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.git.RepoRefCache; +import com.google.gerrit.server.notedb.ChangeBundle; +import com.google.gerrit.server.notedb.ChangeBundleReader; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.NoteDbChangeState; import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; @@ -78,7 +86,8 @@ public class NoteDbPrimaryIT extends AbstractDaemonTest { } @Inject private AllUsersName allUsers; - + @Inject private ChangeBundleReader bundleReader; + @Inject private CommentsUtil commentsUtil; @Inject private TestChangeRebuilderWrapper rebuilderWrapper; private PrimaryStorageMigrator migrator; @@ -368,6 +377,69 @@ public class NoteDbPrimaryIT extends AbstractDaemonTest { assertNoteDbPrimary(id); } + @Test + public void rebuildReviewDb() throws Exception { + Change c = createChange().getChange().change(); + Change.Id id = c.getId(); + + CommentInput cin = new CommentInput(); + cin.line = 1; + cin.message = "Published comment"; + ReviewInput rin = ReviewInput.approve(); + rin.comments = ImmutableMap.of(PushOneCommit.FILE_NAME, ImmutableList.of(cin)); + gApi.changes().id(id.get()).current().review(ReviewInput.approve()); + + DraftInput din = new DraftInput(); + din.path = PushOneCommit.FILE_NAME; + din.line = 1; + din.message = "Draft comment"; + gApi.changes().id(id.get()).current().createDraft(din); + gApi.changes().id(id.get()).current().review(ReviewInput.approve()); + gApi.changes().id(id.get()).current().createDraft(din); + + assertThat(db.changeMessages().byChange(id)).isNotEmpty(); + assertThat(db.patchSets().byChange(id)).isNotEmpty(); + assertThat(db.patchSetApprovals().byChange(id)).isNotEmpty(); + assertThat(db.patchComments().byChange(id)).isNotEmpty(); + + ChangeBundle noteDbBundle = + ChangeBundle.fromNotes(commentsUtil, notesFactory.create(db, project, id)); + + setNoteDbPrimary(id); + + db.changeMessages().delete(db.changeMessages().byChange(id)); + db.patchSets().delete(db.patchSets().byChange(id)); + db.patchSetApprovals().delete(db.patchSetApprovals().byChange(id)); + db.patchComments().delete(db.patchComments().byChange(id)); + ChangeMessage bogusMessage = + ChangeMessagesUtil.newMessage( + c.currentPatchSetId(), + identifiedUserFactory.create(admin.getId()), + TimeUtil.nowTs(), + "some message", + null); + db.changeMessages().insert(Collections.singleton(bogusMessage)); + + rebuilderWrapper.rebuildReviewDb(db, project, id); + + assertThat(db.changeMessages().byChange(id)).isNotEmpty(); + assertThat(db.patchSets().byChange(id)).isNotEmpty(); + assertThat(db.patchSetApprovals().byChange(id)).isNotEmpty(); + assertThat(db.patchComments().byChange(id)).isNotEmpty(); + + ChangeBundle reviewDbBundle = bundleReader.fromReviewDb(ReviewDbUtil.unwrapDb(db), id); + assertThat(reviewDbBundle.differencesFrom(noteDbBundle)).isEmpty(); + } + + @Test + public void rebuildReviewDbRequiresNoteDbPrimary() throws Exception { + Change.Id id = createChange().getChange().getId(); + + exception.expect(OrmException.class); + exception.expectMessage("primary storage of " + id + " is REVIEW_DB"); + rebuilderWrapper.rebuildReviewDb(db, project, id); + } + private void setNoteDbPrimary(Change.Id id) throws Exception { Change c = db.changes().get(id); assertThat(c).named("change " + id).isNotNull(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java index 0b01e148ec..d249689f8a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java @@ -19,6 +19,7 @@ import com.google.common.cache.CacheBuilder; import com.google.gerrit.extensions.config.FactoryModule; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change.Id; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.notedb.NoteDbUpdateManager.Result; import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder; @@ -95,6 +96,11 @@ public class NoteDbModule extends FactoryModule { public void buildUpdates(NoteDbUpdateManager manager, ChangeBundle bundle) { // Do nothing. } + + @Override + public void rebuildReviewDb(ReviewDb db, Project.NameKey project, Id changeId) { + // Do nothing. + } }); bind(new TypeLiteral>() {}) .annotatedWith(Names.named(ChangeNotesCache.CACHE_NAME)) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/TestChangeRebuilderWrapper.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/TestChangeRebuilderWrapper.java index 4ee84dce8d..11fef24ec2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/TestChangeRebuilderWrapper.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/TestChangeRebuilderWrapper.java @@ -16,6 +16,8 @@ package com.google.gerrit.server.notedb; import com.google.common.annotations.VisibleForTesting; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Change.Id; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.notedb.NoteDbUpdateManager.Result; import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder; @@ -111,4 +113,13 @@ public class TestChangeRebuilderWrapper extends ChangeRebuilder { // Don't check for manual failure; that happens in execute(). delegate.buildUpdates(manager, bundle); } + + @Override + public void rebuildReviewDb(ReviewDb db, Project.NameKey project, Id changeId) + throws OrmException { + if (failNextUpdate.getAndSet(false)) { + throw new OrmException("Update failed"); + } + delegate.rebuildReviewDb(db, project, changeId); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilder.java index c8d9bb37b5..6f9090f0bd 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilder.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.notedb.rebuild; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.notedb.ChangeBundle; import com.google.gerrit.server.notedb.NoteDbUpdateManager; @@ -54,6 +55,17 @@ public abstract class ChangeRebuilder { }); } + /** + * Rebuild ReviewDb contents by copying from NoteDb. + * + *

Requires NoteDb to be the primary storage for the change. + */ + public abstract void rebuildReviewDb(ReviewDb db, Project.NameKey project, Change.Id changeId) + throws OrmException; + + // In the following methods "rebuilding" always refers to copying the state + // from ReviewDb to NoteDb, i.e. assuming ReviewDb is the primary storage. + public abstract Result rebuild(ReviewDb db, Change.Id changeId) throws IOException, OrmException; public abstract Result rebuildEvenIfReadOnly(ReviewDb db, Change.Id changeId) 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 10f4dd8c16..4ba3a88a61 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 @@ -23,6 +23,7 @@ import static java.util.concurrent.TimeUnit.SECONDS; import static java.util.stream.Collectors.toList; import com.google.common.base.Splitter; +import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ListMultimap; @@ -41,6 +42,7 @@ import com.google.gerrit.reviewdb.client.PatchLineComment; import com.google.gerrit.reviewdb.client.PatchLineComment.Status; 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.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDbUtil; @@ -67,6 +69,8 @@ import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.ProjectCache; +import com.google.gwtorm.client.Key; +import com.google.gwtorm.server.Access; import com.google.gwtorm.server.AtomicUpdate; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.SchemaFactory; @@ -74,6 +78,7 @@ import com.google.inject.Inject; import java.io.IOException; import java.sql.Timestamp; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.Iterator; @@ -112,7 +117,9 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { private final ChangeBundleReader bundleReader; private final ChangeDraftUpdate.Factory draftUpdateFactory; private final ChangeNoteUtil changeNoteUtil; + private final ChangeNotes.Factory notesFactory; private final ChangeUpdate.Factory updateFactory; + private final CommentsUtil commentsUtil; private final NoteDbUpdateManager.Factory updateManagerFactory; private final NotesMigration migration; private final PatchListCache patchListCache; @@ -130,7 +137,9 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { ChangeBundleReader bundleReader, ChangeDraftUpdate.Factory draftUpdateFactory, ChangeNoteUtil changeNoteUtil, + ChangeNotes.Factory notesFactory, ChangeUpdate.Factory updateFactory, + CommentsUtil commentsUtil, NoteDbUpdateManager.Factory updateManagerFactory, NotesMigration migration, PatchListCache patchListCache, @@ -143,7 +152,9 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { this.bundleReader = bundleReader; this.draftUpdateFactory = draftUpdateFactory; this.changeNoteUtil = changeNoteUtil; + this.notesFactory = notesFactory; this.updateFactory = updateFactory; + this.commentsUtil = commentsUtil; this.updateManagerFactory = updateManagerFactory; this.migration = migration; this.patchListCache = patchListCache; @@ -613,4 +624,45 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { update.setBranch(change.getDest().get()); update.setSubject(change.getOriginalSubject()); } + + @Override + public void rebuildReviewDb(ReviewDb db, Project.NameKey project, Change.Id changeId) + throws OrmException { + // TODO(dborowitz): Fail fast if changes tables are disabled in ReviewDb. + ChangeNotes notes = notesFactory.create(db, project, changeId); + ChangeBundle bundle = ChangeBundle.fromNotes(commentsUtil, notes); + + db = ReviewDbUtil.unwrapDb(db); + db.changes().beginTransaction(changeId); + try { + Change c = db.changes().get(changeId); + PrimaryStorage ps = PrimaryStorage.of(c); + if (ps != PrimaryStorage.NOTE_DB) { + throw new OrmException("primary storage of " + changeId + " is " + ps); + } + db.changes().upsert(Collections.singleton(c)); + putExactlyEntities( + db.changeMessages(), db.changeMessages().byChange(c.getId()), bundle.getChangeMessages()); + putExactlyEntities(db.patchSets(), db.patchSets().byChange(c.getId()), bundle.getPatchSets()); + putExactlyEntities( + db.patchSetApprovals(), + db.patchSetApprovals().byChange(c.getId()), + bundle.getPatchSetApprovals()); + putExactlyEntities( + db.patchComments(), + db.patchComments().byChange(c.getId()), + bundle.getPatchLineComments()); + db.commit(); + } finally { + db.rollback(); + } + } + + private static > void putExactlyEntities( + Access access, Iterable existing, Collection ents) throws OrmException { + Set toKeep = access.toMap(ents).keySet(); + access.delete( + FluentIterable.from(existing).filter(e -> !toKeep.contains(access.primaryKey(e)))); + access.upsert(ents); + } } From 24d3106b00f7a9a94ca9ad018749ab3dc9316420 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 16 Feb 2017 16:59:44 -0500 Subject: [PATCH 2/4] AbstractChangeUpdate: Clarify when notes can be null Change-Id: I3330e91f66d9fd60e1e6f3f424c76db28735036c --- .../google/gerrit/server/notedb/AbstractChangeUpdate.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java index 8df9bd6fd8..93858543e7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java @@ -133,6 +133,14 @@ public abstract class AbstractChangeUpdate { return change.getId(); } + /** + * @return notes for the state of this change prior to this update. If this update is part of a + * series managed by a {@link NoteDbUpdateManager}, then this reflects the state prior to the + * first update in the series. A null return value can only happen when the change is being + * rebuilt from NoteDb. A change that is in the process of being created will result in a + * non-null return value from this method, but a null return value from {@link + * ChangeNotes#getRevision()}. + */ @Nullable public ChangeNotes getNotes() { return notes; From cfb8e840ae23ca5842c825c21250ac7ecd41d85b Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 16 Feb 2017 13:05:53 -0500 Subject: [PATCH 3/4] Support read-only lease in NoteDb meta graph To support reverse migration from NoteDb -> ReviewDb, we need to do a similar thing to what we did with NoteDbChangeState, except in this case NoteDb is the source of truth, so we need to record the lease in NoteDb. Let a determined caller clear a lease by setting the readOnlyUntil to the special timestamp of 0. There is no assurance that the caller is the same one that holds the lease, but this code is going to be used in exactly one place. This does pollute the meta graph with implementation details of the migration, which we didn't really want to do. We can erase our tracks later by rebuilding the change again once ReviewDb is primary again. Change-Id: Ia019fb47e1c1edf719354d0da6bbfe22429f3eb1 --- .../server/notedb/AbstractChangeUpdate.java | 22 +++++- .../server/notedb/ChangeDraftUpdate.java | 6 ++ .../gerrit/server/notedb/ChangeNoteUtil.java | 1 + .../gerrit/server/notedb/ChangeNotes.java | 5 ++ .../server/notedb/ChangeNotesParser.java | 26 ++++++- .../server/notedb/ChangeNotesState.java | 12 ++- .../gerrit/server/notedb/ChangeUpdate.java | 34 ++++++++- .../server/notedb/NoteDbChangeState.java | 2 +- .../server/notedb/RobotCommentUpdate.java | 6 ++ .../gerrit/server/notedb/ChangeNotesTest.java | 73 +++++++++++++++++++ 10 files changed, 179 insertions(+), 8 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java index 93858543e7..a5dc81e979 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java @@ -29,8 +29,10 @@ import com.google.gerrit.server.InternalUser; import com.google.gerrit.server.project.ChangeControl; import com.google.gwtorm.server.OrmException; import java.io.IOException; +import java.sql.Timestamp; import java.util.Date; import org.eclipse.jgit.lib.CommitBuilder; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; @@ -47,15 +49,17 @@ public abstract class AbstractChangeUpdate { protected final Account.Id realAccountId; protected final PersonIdent authorIdent; protected final Date when; + private final long readOnlySkewMs; @Nullable private final ChangeNotes notes; private final Change change; - private final PersonIdent serverIdent; + protected final PersonIdent serverIdent; protected PatchSet.Id psId; private ObjectId result; protected AbstractChangeUpdate( + Config cfg, NotesMigration migration, ChangeControl ctl, PersonIdent serverIdent, @@ -73,9 +77,11 @@ public abstract class AbstractChangeUpdate { this.realAccountId = realAccountId != null ? realAccountId : accountId; this.authorIdent = ident(noteUtil, serverIdent, anonymousCowardName, ctl.getUser(), when); this.when = when; + this.readOnlySkewMs = NoteDbChangeState.getReadOnlySkew(cfg); } protected AbstractChangeUpdate( + Config cfg, NotesMigration migration, ChangeNoteUtil noteUtil, PersonIdent serverIdent, @@ -99,6 +105,7 @@ public abstract class AbstractChangeUpdate { this.realAccountId = realAccountId; this.authorIdent = authorIdent; this.when = when; + this.readOnlySkewMs = NoteDbChangeState.getReadOnlySkew(cfg); } private static void checkUserType(CurrentUser user) { @@ -214,6 +221,7 @@ public abstract class AbstractChangeUpdate { // to actually store. checkArgument(rw.getObjectReader().getCreatedFromInserter() == ins); + checkNotReadOnly(); ObjectId z = ObjectId.zeroId(); CommitBuilder cb = applyImpl(rw, ins, curr); if (cb == null) { @@ -241,6 +249,18 @@ public abstract class AbstractChangeUpdate { return result; } + protected void checkNotReadOnly() throws OrmException { + ChangeNotes notes = getNotes(); + if (notes == null) { + // Can only happen during ChangeRebuilder, which will never include a read-only lease. + return; + } + Timestamp until = notes.getReadOnlyUntil(); + if (until != null && NoteDbChangeState.timeForReadOnlyCheck(readOnlySkewMs).before(until)) { + throw new OrmException("change " + notes.getChangeId() + " is read-only until " + until); + } + } + /** * Create a commit containing the contents of this update. * diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java index 74ffb96ee4..428faefa18 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java @@ -29,6 +29,7 @@ import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.AnonymousCowardName; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gwtorm.server.OrmException; import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.AssistedInject; @@ -42,6 +43,7 @@ import java.util.Map; import java.util.Set; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.CommitBuilder; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.PersonIdent; @@ -91,6 +93,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { @AssistedInject private ChangeDraftUpdate( + @GerritServerConfig Config cfg, @GerritPersonIdent PersonIdent serverIdent, @AnonymousCowardName String anonymousCowardName, NotesMigration migration, @@ -102,6 +105,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { @Assisted PersonIdent authorIdent, @Assisted Date when) { super( + cfg, migration, noteUtil, serverIdent, @@ -117,6 +121,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { @AssistedInject private ChangeDraftUpdate( + @GerritServerConfig Config cfg, @GerritPersonIdent PersonIdent serverIdent, @AnonymousCowardName String anonymousCowardName, NotesMigration migration, @@ -128,6 +133,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { @Assisted PersonIdent authorIdent, @Assisted Date when) { super( + cfg, migration, noteUtil, serverIdent, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java index 8d61c36b08..c8489878be 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java @@ -73,6 +73,7 @@ public class ChangeNoteUtil { public static final FooterKey FOOTER_PATCH_SET = new FooterKey("Patch-set"); public static final FooterKey FOOTER_PATCH_SET_DESCRIPTION = new FooterKey("Patch-set-description"); + public static final FooterKey FOOTER_READ_ONLY_UNTIL = new FooterKey("Read-only-until"); public static final FooterKey FOOTER_REAL_USER = new FooterKey("Real-user"); public static final FooterKey FOOTER_STATUS = new FooterKey("Status"); public static final FooterKey FOOTER_SUBJECT = new FooterKey("Subject"); 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 b15874ddb8..d9722b7768 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 @@ -63,6 +63,7 @@ import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; +import java.sql.Timestamp; import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; @@ -558,6 +559,10 @@ public class ChangeNotes extends AbstractChangeNotes { return checkNotNull(getPatchSets().get(psId), "missing current patch set %s", psId.get()); } + Timestamp getReadOnlyUntil() { + return state.readOnlyUntil(); + } + @Override protected void onLoad(LoadHandle handle) throws NoSuchChangeException, IOException, ConfigInvalidException { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java index 2a9d985359..dac999c57a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java @@ -24,6 +24,7 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_HASHTAGS; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_LABEL; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PATCH_SET; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PATCH_SET_DESCRIPTION; +import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_READ_ONLY_UNTIL; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_REAL_USER; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_STATUS; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBJECT; @@ -68,6 +69,7 @@ import com.google.gerrit.server.util.LabelVote; import java.io.IOException; import java.nio.charset.Charset; import java.sql.Timestamp; +import java.text.ParseException; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -76,6 +78,7 @@ import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Objects; import java.util.Optional; @@ -89,6 +92,7 @@ import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.notes.NoteMap; import org.eclipse.jgit.revwalk.FooterKey; +import org.eclipse.jgit.util.GitDateParser; import org.eclipse.jgit.util.RawParseUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -152,6 +156,7 @@ class ChangeNotesParser { private String submissionId; private String tag; private RevisionNoteMap revisionNoteMap; + private Timestamp readOnlyUntil; ChangeNotesParser( Change.Id changeId, @@ -232,7 +237,8 @@ class ChangeNotesParser { submitRecords, buildAllMessages(), buildMessagesByPatchSet(), - comments); + comments, + readOnlyUntil); } private PatchSet.Id buildCurrentPatchSetId() { @@ -369,6 +375,10 @@ class ChangeNotesParser { // behavior. } + if (readOnlyUntil == null) { + parseReadOnlyUntil(commit); + } + if (lastUpdatedOn == null || ts.after(lastUpdatedOn)) { lastUpdatedOn = ts; } @@ -900,6 +910,20 @@ class ChangeNotesParser { } } + private void parseReadOnlyUntil(ChangeNotesCommit commit) throws ConfigInvalidException { + String raw = parseOneFooter(commit, FOOTER_READ_ONLY_UNTIL); + if (raw == null) { + return; + } + try { + readOnlyUntil = new Timestamp(GitDateParser.parse(raw, null, Locale.US).getTime()); + } catch (ParseException e) { + ConfigInvalidException cie = invalidFooter(FOOTER_READ_ONLY_UNTIL, raw); + cie.initCause(e); + throw cie; + } + } + private void pruneReviewers() { Iterator> rit = reviewers.cellSet().iterator(); 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 62df01b811..7b25bbd686 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 @@ -70,7 +70,8 @@ public abstract class ChangeNotesState { ImmutableList.of(), ImmutableList.of(), ImmutableListMultimap.of(), - ImmutableListMultimap.of()); + ImmutableListMultimap.of(), + null); } static ChangeNotesState create( @@ -98,7 +99,8 @@ public abstract class ChangeNotesState { List submitRecords, List allChangeMessages, ListMultimap changeMessagesByPatchSet, - ListMultimap publishedComments) { + ListMultimap publishedComments, + @Nullable Timestamp readOnlyUntil) { if (hashtags == null) { hashtags = ImmutableSet.of(); } @@ -128,7 +130,8 @@ public abstract class ChangeNotesState { ImmutableList.copyOf(submitRecords), ImmutableList.copyOf(allChangeMessages), ImmutableListMultimap.copyOf(changeMessagesByPatchSet), - ImmutableListMultimap.copyOf(publishedComments)); + ImmutableListMultimap.copyOf(publishedComments), + readOnlyUntil); } /** @@ -206,6 +209,9 @@ public abstract class ChangeNotesState { abstract ImmutableListMultimap publishedComments(); + @Nullable + abstract Timestamp readOnlyUntil(); + Change newChange(Project.NameKey project) { ChangeColumns c = checkNotNull(columns(), "columns are required"); Change change = 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 c241965fcc..7af0cb4766 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 @@ -29,6 +29,7 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_HASHTAGS; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_LABEL; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PATCH_SET; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PATCH_SET_DESCRIPTION; +import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_READ_ONLY_UNTIL; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_REAL_USER; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_STATUS; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBJECT; @@ -58,6 +59,7 @@ import com.google.gerrit.reviewdb.client.RobotComment; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.config.AnonymousCowardName; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.util.LabelVote; @@ -67,6 +69,7 @@ import com.google.gwtorm.server.OrmException; import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.AssistedInject; import java.io.IOException; +import java.sql.Timestamp; import java.util.ArrayList; import java.util.Comparator; import java.util.Date; @@ -79,6 +82,7 @@ import java.util.Optional; import java.util.Set; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.CommitBuilder; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.PersonIdent; @@ -144,12 +148,14 @@ public class ChangeUpdate extends AbstractChangeUpdate { private boolean isAllowWriteToNewtRef; private String psDescription; private boolean currentPatchSet; + private Timestamp readOnlyUntil; private ChangeDraftUpdate draftUpdate; private RobotCommentUpdate robotCommentUpdate; @AssistedInject private ChangeUpdate( + @GerritServerConfig Config cfg, @GerritPersonIdent PersonIdent serverIdent, @AnonymousCowardName String anonymousCowardName, NotesMigration migration, @@ -161,6 +167,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { @Assisted ChangeControl ctl, ChangeNoteUtil noteUtil) { this( + cfg, serverIdent, anonymousCowardName, migration, @@ -176,6 +183,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { @AssistedInject private ChangeUpdate( + @GerritServerConfig Config cfg, @GerritPersonIdent PersonIdent serverIdent, @AnonymousCowardName String anonymousCowardName, NotesMigration migration, @@ -188,6 +196,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { @Assisted Date when, ChangeNoteUtil noteUtil) { this( + cfg, serverIdent, anonymousCowardName, migration, @@ -212,6 +221,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { @AssistedInject private ChangeUpdate( + @GerritServerConfig Config cfg, @GerritPersonIdent PersonIdent serverIdent, @AnonymousCowardName String anonymousCowardName, NotesMigration migration, @@ -223,7 +233,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { @Assisted Date when, @Assisted Comparator labelNameComparator, ChangeNoteUtil noteUtil) { - super(migration, ctl, serverIdent, anonymousCowardName, noteUtil, when); + super(cfg, migration, ctl, serverIdent, anonymousCowardName, noteUtil, when); this.accountCache = accountCache; this.draftUpdateFactory = draftUpdateFactory; this.robotCommentUpdateFactory = robotCommentUpdateFactory; @@ -233,6 +243,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { @AssistedInject private ChangeUpdate( + @GerritServerConfig Config cfg, @GerritPersonIdent PersonIdent serverIdent, @AnonymousCowardName String anonymousCowardName, NotesMigration migration, @@ -248,6 +259,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { @Assisted Date when, @Assisted Comparator labelNameComparator) { super( + cfg, migration, noteUtil, serverIdent, @@ -695,6 +707,10 @@ public class ChangeUpdate extends AbstractChangeUpdate { addIdent(msg, realAccountId).append('\n'); } + if (readOnlyUntil != null) { + addFooter(msg, FOOTER_READ_ONLY_UNTIL, ChangeNoteUtil.formatTime(serverIdent, readOnlyUntil)); + } + cb.setMessage(msg.toString()); try { ObjectId treeId = storeRevisionNotes(rw, ins, curr); @@ -740,7 +756,8 @@ public class ChangeUpdate extends AbstractChangeUpdate { && groups == null && tag == null && psDescription == null - && !currentPatchSet; + && !currentPatchSet + && readOnlyUntil == null; } ChangeDraftUpdate getDraftUpdate() { @@ -760,6 +777,10 @@ public class ChangeUpdate extends AbstractChangeUpdate { return isAllowWriteToNewtRef; } + void setReadOnlyUntil(Timestamp readOnlyUntil) { + this.readOnlyUntil = readOnlyUntil; + } + private static StringBuilder addFooter(StringBuilder sb, FooterKey footer) { return sb.append(footer.getName()).append(": "); } @@ -782,4 +803,13 @@ public class ChangeUpdate extends AbstractChangeUpdate { sb.append('>'); return sb; } + + @Override + protected void checkNotReadOnly() throws OrmException { + // Allow setting Read-only-until to 0 to release an existing lease. + if (readOnlyUntil != null && readOnlyUntil.getTime() == 0) { + return; + } + super.checkNotReadOnly(); + } } 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 1350a1f460..fef7fdf15d 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 @@ -301,7 +301,7 @@ public class NoteDbChangeState { return cfg.getTimeUnit("notedb", null, "maxTimestampSkew", 1000, TimeUnit.MILLISECONDS); } - private static Timestamp timeForReadOnlyCheck(long skewMs) { + static Timestamp timeForReadOnlyCheck(long skewMs) { // Subtract some slop in case the machine that set the change's read-only // lease has a clock behind ours. return new Timestamp(TimeUtil.nowMs() - skewMs); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RobotCommentUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RobotCommentUpdate.java index 9ce4b54773..82593eb49c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RobotCommentUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RobotCommentUpdate.java @@ -26,6 +26,7 @@ import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.client.RobotComment; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.config.AnonymousCowardName; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gwtorm.server.OrmException; import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.AssistedInject; @@ -38,6 +39,7 @@ import java.util.Map; import java.util.Set; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.CommitBuilder; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.PersonIdent; @@ -73,6 +75,7 @@ public class RobotCommentUpdate extends AbstractChangeUpdate { @AssistedInject private RobotCommentUpdate( + @GerritServerConfig Config cfg, @GerritPersonIdent PersonIdent serverIdent, @AnonymousCowardName String anonymousCowardName, NotesMigration migration, @@ -83,6 +86,7 @@ public class RobotCommentUpdate extends AbstractChangeUpdate { @Assisted PersonIdent authorIdent, @Assisted Date when) { super( + cfg, migration, noteUtil, serverIdent, @@ -97,6 +101,7 @@ public class RobotCommentUpdate extends AbstractChangeUpdate { @AssistedInject private RobotCommentUpdate( + @GerritServerConfig Config cfg, @GerritPersonIdent PersonIdent serverIdent, @AnonymousCowardName String anonymousCowardName, NotesMigration migration, @@ -107,6 +112,7 @@ public class RobotCommentUpdate extends AbstractChangeUpdate { @Assisted PersonIdent authorIdent, @Assisted Date when) { super( + cfg, migration, noteUtil, serverIdent, diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java index a4990c1ad2..f914ef8b3c 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.notedb; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assert_; import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef; import static com.google.gerrit.reviewdb.client.RefNames.refsDraftComments; import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC; @@ -51,6 +52,7 @@ import com.google.gerrit.server.config.GerritServerId; import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk; import com.google.gerrit.server.util.RequestId; import com.google.gerrit.testutil.TestChanges; +import com.google.gerrit.testutil.TestTimeUtil; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import java.sql.Timestamp; @@ -58,6 +60,7 @@ import java.util.ArrayList; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.concurrent.TimeUnit; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; @@ -3193,6 +3196,76 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { assertThat(newNotes(c).getChange().currentPatchSetId().get()).isEqualTo(2); } + @Test + public void readOnlyUntilExpires() throws Exception { + Change c = newChange(); + ChangeUpdate update = newUpdate(c, changeOwner); + Timestamp until = new Timestamp(TimeUtil.nowMs() + 10000); + update.setReadOnlyUntil(until); + update.commit(); + + update = newUpdate(c, changeOwner); + update.setTopic("failing-topic"); + try { + update.commit(); + assert_().fail("expected OrmException"); + } catch (OrmException e) { + assertThat(e.getMessage()).contains("read-only until"); + } + + ChangeNotes notes = newNotes(c); + assertThat(notes.getChange().getTopic()).isNotEqualTo("failing-topic"); + assertThat(notes.getReadOnlyUntil()).isEqualTo(until); + + TestTimeUtil.incrementClock(30, TimeUnit.SECONDS); + update = newUpdate(c, changeOwner); + update.setTopic("succeeding-topic"); + update.commit(); + + // Write succeeded; lease still exists, even though it's expired. + notes = newNotes(c); + assertThat(notes.getChange().getTopic()).isEqualTo("succeeding-topic"); + assertThat(notes.getReadOnlyUntil()).isEqualTo(until); + + // New lease takes precedence. + update = newUpdate(c, changeOwner); + until = new Timestamp(TimeUtil.nowMs() + 10000); + update.setReadOnlyUntil(until); + update.commit(); + assertThat(newNotes(c).getReadOnlyUntil()).isEqualTo(until); + } + + @Test + public void readOnlyUntilCleared() throws Exception { + Change c = newChange(); + ChangeUpdate update = newUpdate(c, changeOwner); + Timestamp until = new Timestamp(TimeUtil.nowMs() + TimeUnit.DAYS.toMillis(30)); + update.setReadOnlyUntil(until); + update.commit(); + + update = newUpdate(c, changeOwner); + update.setTopic("failing-topic"); + try { + update.commit(); + assert_().fail("expected OrmException"); + } catch (OrmException e) { + assertThat(e.getMessage()).contains("read-only until"); + } + + // Sentinel timestamp of 0 can be written to clear lease. + update = newUpdate(c, changeOwner); + update.setReadOnlyUntil(new Timestamp(0)); + update.commit(); + + update = newUpdate(c, changeOwner); + update.setTopic("succeeding-topic"); + update.commit(); + + ChangeNotes notes = newNotes(c); + assertThat(notes.getChange().getTopic()).isEqualTo("succeeding-topic"); + assertThat(notes.getReadOnlyUntil()).isEqualTo(new Timestamp(0)); + } + private boolean testJson() { return noteUtil.getWriteJson(); } From 6ff2aef6f83021c1e2c91820e9bb91504c659dc8 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 16 Feb 2017 15:53:34 -0500 Subject: [PATCH 4/4] Support migrating PrimaryStorage from NoteDb -> ReviewDb Change-Id: I14fb7be482b0cfbbdbb6eefdde535f37941fd85b --- .../server/notedb/NoteDbPrimaryIT.java | 61 +++++- .../gerrit/server/notedb/ChangeNotes.java | 3 +- .../server/notedb/PrimaryStorageMigrator.java | 185 +++++++++++++++++- 3 files changed, 240 insertions(+), 9 deletions(-) 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 index 542029ae26..da6aef667b 100644 --- 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 @@ -17,6 +17,7 @@ package com.google.gerrit.acceptance.server.notedb; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assert_; import static com.google.common.truth.TruthJUnit.assume; +import static com.google.gerrit.server.notedb.ChangeNoteUtil.formatTime; import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB; import static java.util.concurrent.TimeUnit.DAYS; import static java.util.concurrent.TimeUnit.MILLISECONDS; @@ -45,18 +46,23 @@ import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; +import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDbUtil; import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.CommentsUtil; +import com.google.gerrit.server.InternalUser; import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.RepoRefCache; import com.google.gerrit.server.notedb.ChangeBundle; import com.google.gerrit.server.notedb.ChangeBundleReader; 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.PrimaryStorageMigrator; import com.google.gerrit.server.notedb.TestChangeRebuilderWrapper; +import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.testutil.ConfigSuite; import com.google.gerrit.testutil.NoteDbMode; import com.google.gerrit.testutil.TestTimeUtil; @@ -70,7 +76,10 @@ import java.util.Collections; import java.util.List; import java.util.Optional; import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevWalk; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -86,9 +95,13 @@ public class NoteDbPrimaryIT extends AbstractDaemonTest { } @Inject private AllUsersName allUsers; + @Inject private BatchUpdate.Factory batchUpdateFactory; @Inject private ChangeBundleReader bundleReader; @Inject private CommentsUtil commentsUtil; @Inject private TestChangeRebuilderWrapper rebuilderWrapper; + @Inject private ChangeControl.GenericFactory changeControlFactory; + @Inject private ChangeUpdate.Factory updateFactory; + @Inject private InternalUser.Factory internalUserFactory; private PrimaryStorageMigrator migrator; @@ -103,7 +116,17 @@ public class NoteDbPrimaryIT extends AbstractDaemonTest { private PrimaryStorageMigrator newMigrator( @Nullable Retryer ensureRebuiltRetryer) { return new PrimaryStorageMigrator( - cfg, Providers.of(db), repoManager, allUsers, rebuilderWrapper, ensureRebuiltRetryer); + cfg, + Providers.of(db), + repoManager, + allUsers, + rebuilderWrapper, + ensureRebuiltRetryer, + changeControlFactory, + queryProvider, + updateFactory, + internalUserFactory, + batchUpdateFactory); } @After @@ -440,6 +463,42 @@ public class NoteDbPrimaryIT extends AbstractDaemonTest { rebuilderWrapper.rebuildReviewDb(db, project, id); } + @Test + public void migrateBackToReviewDbPrimary() throws Exception { + Change c = createChange().getChange().change(); + Change.Id id = c.getId(); + + migrator.migrateToNoteDbPrimary(id); + assertNoteDbPrimary(id); + + gApi.changes().id(id.get()).topic("new-topic"); + assertThat(gApi.changes().id(id.get()).topic()).isEqualTo("new-topic"); + assertThat(db.changes().get(id).getTopic()).isNotEqualTo("new-topic"); + + migrator.migrateToReviewDbPrimary(id, null); + ObjectId metaId; + try (Repository repo = repoManager.openRepository(c.getProject()); + RevWalk rw = new RevWalk(repo)) { + metaId = repo.exactRef(RefNames.changeMetaRef(id)).getObjectId(); + RevCommit commit = rw.parseCommit(metaId); + rw.parseBody(commit); + assertThat(commit.getFullMessage()) + .contains("Read-only-until: " + formatTime(serverIdent.get(), new Timestamp(0))); + } + NoteDbChangeState state = NoteDbChangeState.parse(db.changes().get(id)); + assertThat(state.getPrimaryStorage()).isEqualTo(PrimaryStorage.REVIEW_DB); + assertThat(state.getChangeMetaId()).isEqualTo(metaId); + assertThat(gApi.changes().id(id.get()).topic()).isEqualTo("new-topic"); + assertThat(db.changes().get(id).getTopic()).isEqualTo("new-topic"); + + ChangeNotes notes = notesFactory.create(db, project, id); + assertThat(notes.getRevision()).isEqualTo(metaId); // No rebuilding, change was up to date. + assertThat(notes.getReadOnlyUntil()).isNotNull(); + + gApi.changes().id(id.get()).topic("reviewdb-topic"); + assertThat(db.changes().get(id).getTopic()).isEqualTo("reviewdb-topic"); + } + private void setNoteDbPrimary(Change.Id id) throws Exception { Change c = db.changes().get(id); assertThat(c).named("change " + id).isNotNull(); 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 d9722b7768..13d891b129 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 @@ -559,7 +559,8 @@ public class ChangeNotes extends AbstractChangeNotes { return checkNotNull(getPatchSets().get(psId), "missing current patch set %s", psId.get()); } - Timestamp getReadOnlyUntil() { + @VisibleForTesting + public Timestamp getReadOnlyUntil() { return state.readOnlyUntil(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/PrimaryStorageMigrator.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/PrimaryStorageMigrator.java index 10345c8d2c..0995c2d375 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/PrimaryStorageMigrator.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/PrimaryStorageMigrator.java @@ -26,18 +26,32 @@ import com.github.rholder.retry.StopStrategies; import com.github.rholder.retry.WaitStrategies; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Stopwatch; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.TimeUtil; +import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDbUtil; +import com.google.gerrit.server.InternalUser; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gerrit.server.git.BatchUpdate; +import com.google.gerrit.server.git.BatchUpdate.ChangeContext; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.RepoRefCache; +import com.google.gerrit.server.git.UpdateException; +import com.google.gerrit.server.index.change.ChangeField; import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; +import com.google.gerrit.server.notedb.NoteDbChangeState.RefState; import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder; +import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gwtorm.server.AtomicUpdate; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmRuntimeException; @@ -46,11 +60,17 @@ import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; import java.sql.Timestamp; +import java.util.List; import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.TreeSet; import java.util.concurrent.ExecutionException; import java.util.concurrent.atomic.AtomicBoolean; import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -60,10 +80,15 @@ import org.slf4j.LoggerFactory; public class PrimaryStorageMigrator { private static final Logger log = LoggerFactory.getLogger(PrimaryStorageMigrator.class); - private final Provider db; - private final GitRepositoryManager repoManager; private final AllUsersName allUsers; + private final BatchUpdate.Factory batchUpdateFactory; + private final ChangeControl.GenericFactory changeControlFactory; private final ChangeRebuilder rebuilder; + private final ChangeUpdate.Factory updateFactory; + private final GitRepositoryManager repoManager; + private final InternalUser.Factory internalUserFactory; + private final Provider queryProvider; + private final Provider db; private final long skewMs; private final long timeoutMs; @@ -75,8 +100,24 @@ public class PrimaryStorageMigrator { Provider db, GitRepositoryManager repoManager, AllUsersName allUsers, - ChangeRebuilder rebuilder) { - this(cfg, db, repoManager, allUsers, rebuilder, null); + ChangeRebuilder rebuilder, + ChangeControl.GenericFactory changeControlFactory, + Provider queryProvider, + ChangeUpdate.Factory updateFactory, + InternalUser.Factory internalUserFactory, + BatchUpdate.Factory batchUpdateFactory) { + this( + cfg, + db, + repoManager, + allUsers, + rebuilder, + null, + changeControlFactory, + queryProvider, + updateFactory, + internalUserFactory, + batchUpdateFactory); } @VisibleForTesting @@ -86,12 +127,22 @@ public class PrimaryStorageMigrator { GitRepositoryManager repoManager, AllUsersName allUsers, ChangeRebuilder rebuilder, - @Nullable Retryer testEnsureRebuiltRetryer) { + @Nullable Retryer testEnsureRebuiltRetryer, + ChangeControl.GenericFactory changeControlFactory, + Provider queryProvider, + ChangeUpdate.Factory updateFactory, + InternalUser.Factory internalUserFactory, + BatchUpdate.Factory batchUpdateFactory) { this.db = db; this.repoManager = repoManager; this.allUsers = allUsers; this.rebuilder = rebuilder; this.testEnsureRebuiltRetryer = testEnsureRebuiltRetryer; + this.changeControlFactory = changeControlFactory; + this.queryProvider = queryProvider; + this.updateFactory = updateFactory; + this.internalUserFactory = internalUserFactory; + this.batchUpdateFactory = batchUpdateFactory; skewMs = NoteDbChangeState.getReadOnlySkew(cfg); String s = "notedb"; @@ -189,7 +240,7 @@ public class PrimaryStorageMigrator { // failure. Stopwatch sw = Stopwatch.createStarted(); - Change readOnlyChange = setReadOnly(id); // MRO + Change readOnlyChange = setReadOnlyInReviewDb(id); // MRO if (readOnlyChange == null) { return; // Already migrated. } @@ -217,7 +268,7 @@ public class PrimaryStorageMigrator { log.info("Migrated change {} to NoteDb primary in {}ms", id, sw.elapsed(MILLISECONDS)); } - private Change setReadOnly(Change.Id id) throws OrmException { + private Change setReadOnlyInReviewDb(Change.Id id) throws OrmException { AtomicBoolean alreadyMigrated = new AtomicBoolean(false); Change result = db().changes() @@ -316,4 +367,124 @@ public class PrimaryStorageMigrator { private String badState(NoteDbChangeState actual, NoteDbChangeState expected) { return "state changed unexpectedly: " + actual + " != " + expected; } + + public void migrateToReviewDbPrimary(Change.Id id, @Nullable Project.NameKey project) + throws OrmException, IOException { + // Migrating back to ReviewDb primary is much simpler than the original migration to NoteDb + // primary, because when NoteDb is primary, each write only goes to one storage location rather + // than both. We only need to consider whether a concurrent writer (OR) conflicts with the first + // setReadOnlyInNoteDb step (MR) in this method. + // + // If OR wins, then either: + // * MR will set read-only after OR is completed, which is not a concurrent write. + // * MR will fail to set read-only with a lock failure. The caller will have to retry, but the + // change is not in a read-only state, so behavior is not degraded in the meantime. + // + // If MR wins, then either: + // * OR will fail with a read-only exception (via AbstractChangeNotes#apply). + // * OR will fail with a lock failure. + // + // In all of these scenarios, the change is read-only if and only if MR succeeds. + // + // There will be no concurrent writes to ReviewDb for this change until + // setPrimaryStorageReviewDb completes, because ReviewDb writes are not attempted when primary + // storage is NoteDb. After the primary storage changes back, it is possible for subsequent + // NoteDb writes to conflict with the releaseReadOnlyLeaseInNoteDb step, but at this point, + // since ReviewDb is primary, we are back to ignoring them. + Stopwatch sw = Stopwatch.createStarted(); + if (project == null) { + project = getProject(id); + } + ObjectId newMetaId = setReadOnlyInNoteDb(project, id); + rebuilder.rebuildReviewDb(db(), project, id); + setPrimaryStorageReviewDb(id, newMetaId); + releaseReadOnlyLeaseInNoteDb(project, id); + log.info("Migrated change {} to ReviewDb primary in {}ms", id, sw.elapsed(MILLISECONDS)); + } + + private ObjectId setReadOnlyInNoteDb(Project.NameKey project, Change.Id id) + throws OrmException, IOException { + Timestamp now = TimeUtil.nowTs(); + Timestamp until = new Timestamp(now.getTime() + timeoutMs); + ChangeUpdate update = + updateFactory.create( + changeControlFactory.controlFor(db.get(), project, id, internalUserFactory.create())); + update.setReadOnlyUntil(until); + return update.commit(); + } + + private void setPrimaryStorageReviewDb(Change.Id id, ObjectId newMetaId) + throws OrmException, IOException { + ImmutableMap.Builder draftIds = ImmutableMap.builder(); + try (Repository repo = repoManager.openRepository(allUsers)) { + for (Ref draftRef : + repo.getRefDatabase().getRefs(RefNames.refsDraftCommentsPrefix(id)).values()) { + Account.Id accountId = Account.Id.fromRef(draftRef.getName()); + if (accountId != null) { + draftIds.put(accountId, draftRef.getObjectId().copy()); + } + } + } + NoteDbChangeState newState = + new NoteDbChangeState( + id, + PrimaryStorage.REVIEW_DB, + Optional.of(RefState.create(newMetaId, draftIds.build())), + Optional.empty()); + db().changes() + .atomicUpdate( + id, + new AtomicUpdate() { + @Override + public Change update(Change change) { + if (PrimaryStorage.of(change) != PrimaryStorage.NOTE_DB) { + throw new OrmRuntimeException( + "change " + id + " is not NoteDb primary: " + change.getNoteDbState()); + } + change.setNoteDbState(newState.toString()); + return change; + } + }); + } + + private void releaseReadOnlyLeaseInNoteDb(Project.NameKey project, Change.Id id) + throws OrmException { + // Use a BatchUpdate since ReviewDb is primary at this point, so it needs to reflect the update. + try (BatchUpdate bu = + batchUpdateFactory.create( + db.get(), project, internalUserFactory.create(), TimeUtil.nowTs())) { + bu.addOp( + id, + new BatchUpdate.Op() { + @Override + public boolean updateChange(ChangeContext ctx) { + ctx.getUpdate(ctx.getChange().currentPatchSetId()).setReadOnlyUntil(new Timestamp(0)); + return true; + } + }); + bu.execute(); + } catch (RestApiException | UpdateException e) { + throw new OrmException(e); + } + } + + private Project.NameKey getProject(Change.Id id) throws OrmException { + List cds = + queryProvider + .get() + .setRequestedFields(ImmutableSet.of(ChangeField.PROJECT.getName())) + .byLegacyChangeId(id); + Set projects = new TreeSet<>(); + for (ChangeData cd : cds) { + projects.add(cd.project()); + } + if (projects.size() != 1) { + throw new OrmException( + "zero or multiple projects found for change " + + id + + ", must specify project explicitly: " + + projects); + } + return projects.iterator().next(); + } }