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
This commit is contained in:
Dave Borowitz 2017-02-16 13:05:53 -05:00
parent 24d3106b00
commit cfb8e840ae
10 changed files with 179 additions and 8 deletions

@ -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.
*

@ -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,

@ -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");

@ -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<ChangeNotes> {
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 {

@ -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<ChangeRevisionNote> 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<Table.Cell<Account.Id, ReviewerStateInternal, Timestamp>> rit =
reviewers.cellSet().iterator();

@ -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<SubmitRecord> submitRecords,
List<ChangeMessage> allChangeMessages,
ListMultimap<PatchSet.Id, ChangeMessage> changeMessagesByPatchSet,
ListMultimap<RevId, Comment> publishedComments) {
ListMultimap<RevId, Comment> 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<RevId, Comment> publishedComments();
@Nullable
abstract Timestamp readOnlyUntil();
Change newChange(Project.NameKey project) {
ChangeColumns c = checkNotNull(columns(), "columns are required");
Change change =

@ -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<String> 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<String> 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();
}
}

@ -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);

@ -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,

@ -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();
}