opendev/gerrit
Code Issues Proposed changes

Support temporarily read-only changes via NoteDbChangeState

Browse Source 
Add a new piece of data to NoteDbChangeState indicating that a change
is read-only until a specified future time.  We use a timestamp
instead of a separate state to support a migration step where the job
doing the migration might die, so the change automatically becomes
read-write again without manual intervention.

This read-only state only needs to be checked while updating
ReviewDb, which includes an atomic update to the noteDbState field.
There are exactly two locations that do this atomic update:
ChangeRebuilderImpl#execute and BatchUpdate.ChangeTask#call.

We don't bother with attempting to double-check this field when doing
the corresponding NoteDb write, for example from NoteDbUpdateManager.
It's not possible when we're about to write to NoteDb to do a
race-free atomic read of both the read-only state from ReviewDb and
the ref in NoteDb that we're about to update. So, we just let the
NoteDb write attempt proceed. This turns out to not affect the
correctness of the migration process; the race condition is explained
in more detail in the commit that implements the migration process.

Change-Id: If21353582f678b8788285bc2e9f7b50a5c14f6d4
This commit is contained in:
Dave Borowitz
2016-10-24 14:51:22 -04:00
parent f125df6aa6
commit a96742c035
9 changed files with 385 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files

100
gerrit-server/src/test/java/com/google/gerrit/server/notedb/NoteDbChangeStateTest.java
View File

@@ -16,6 +16,7 @@ package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.common.TimeUtil.nowTs;
import static com.google.gerrit.server.notedb.NoteDbChangeState.applyDelta;
import static com.google.gerrit.server.notedb.NoteDbChangeState.parse;
import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.NOTE_DB;
@@ -27,13 +28,19 @@ 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.server.notedb.NoteDbChangeState.Delta;
import com.google.gerrit.server.notedb.NoteDbChangeState.RefState;
import com.google.gerrit.testutil.GerritBaseTests;
import com.google.gerrit.testutil.TestChanges;
import com.google.gerrit.testutil.TestTimeUtil;
import org.eclipse.jgit.lib.ObjectId;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import java.sql.Timestamp;
import java.util.Optional;
import java.util.concurrent.TimeUnit;
/** Unit tests for {@link NoteDbChangeState}. */
public class NoteDbChangeStateTest extends GerritBaseTests {
@@ -44,6 +51,16 @@ public class NoteDbChangeStateTest extends GerritBaseTests {
ObjectId SHA3 =
ObjectId.fromString("badc0feebadc0feebadc0feebadc0feebadc0fee");
@Before
public void setUp() {
TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS);
}
@After
public void tearDown() {
TestTimeUtil.useSystemTime();
}
@Test
public void parseReviewDbWithoutDrafts() {
NoteDbChangeState state = parse(new Change.Id(1), SHA1.name());
@@ -51,6 +68,7 @@ public class NoteDbChangeStateTest extends GerritBaseTests {
assertThat(state.getChangeId()).isEqualTo(new Change.Id(1));
assertThat(state.getChangeMetaId()).isEqualTo(SHA1);
assertThat(state.getDraftIds()).isEmpty();
assertThat(state.getReadOnlyUntil().isPresent()).isFalse();
assertThat(state.toString()).isEqualTo(SHA1.name());
state = parse(new Change.Id(1), "R," + SHA1.name());
@@ -58,6 +76,7 @@ public class NoteDbChangeStateTest extends GerritBaseTests {
assertThat(state.getChangeId()).isEqualTo(new Change.Id(1));
assertThat(state.getChangeMetaId()).isEqualTo(SHA1);
assertThat(state.getDraftIds()).isEmpty();
assertThat(state.getReadOnlyUntil().isPresent()).isFalse();
assertThat(state.toString()).isEqualTo(SHA1.name());
}
@@ -73,6 +92,7 @@ public class NoteDbChangeStateTest extends GerritBaseTests {
assertThat(state.getDraftIds()).containsExactly(
new Account.Id(1001), SHA3,
new Account.Id(2003), SHA2);
assertThat(state.getReadOnlyUntil().isPresent()).isFalse();
assertThat(state.toString()).isEqualTo(expected);
state = parse(new Change.Id(1), "R," + str);
@@ -82,23 +102,47 @@ public class NoteDbChangeStateTest extends GerritBaseTests {
assertThat(state.getDraftIds()).containsExactly(
new Account.Id(1001), SHA3,
new Account.Id(2003), SHA2);
assertThat(state.getReadOnlyUntil().isPresent()).isFalse();
assertThat(state.toString()).isEqualTo(expected);
}
@Test
public void applyDeltaToNullWithNoNewMetaId() {
public void parseReadOnlyUntil() {
Timestamp ts = new Timestamp(12345);
String str = "R=12345," + SHA1.name();
NoteDbChangeState state = parse(new Change.Id(1), str);
assertThat(state.getPrimaryStorage()).isEqualTo(REVIEW_DB);
assertThat(state.getChangeId()).isEqualTo(new Change.Id(1));
assertThat(state.getChangeMetaId()).isEqualTo(SHA1);
assertThat(state.getReadOnlyUntil().get()).isEqualTo(ts);
assertThat(state.toString()).isEqualTo(str);
str = "N=12345";
state = parse(new Change.Id(1), str);
assertThat(state.getPrimaryStorage()).isEqualTo(NOTE_DB);
assertThat(state.getChangeId()).isEqualTo(new Change.Id(1));
assertThat(state.getRefState().isPresent()).isFalse();
assertThat(state.getReadOnlyUntil().get()).isEqualTo(ts);
assertThat(state.toString()).isEqualTo(str);
}
@Test
public void applyDeltaToNullWithNoNewMetaId() throws Exception {
Change c = newChange();
assertThat(c.getNoteDbState()).isNull();
applyDelta(c, Delta.create(c.getId(), noMetaId(), noDrafts()));
assertThat(c.getNoteDbState()).isNull();
applyDelta(c, Delta.create(c.getId(), noMetaId(),
drafts(new Account.Id(1001), zeroId())));
applyDelta(
c,
Delta.create(
c.getId(), noMetaId(),
drafts(new Account.Id(1001), zeroId())));
assertThat(c.getNoteDbState()).isNull();
}
@Test
public void applyDeltaToMetaId() {
public void applyDeltaToMetaId() throws Exception {
Change c = newChange();
applyDelta(c, Delta.create(c.getId(), metaId(SHA1), noDrafts()));
assertThat(c.getNoteDbState()).isEqualTo(SHA1.name());
@@ -116,33 +160,63 @@ public class NoteDbChangeStateTest extends GerritBaseTests {
}
@Test
public void applyDeltaToDrafts() {
public void applyDeltaToDrafts() throws Exception {
Change c = newChange();
applyDelta(c, Delta.create(c.getId(), metaId(SHA1),
drafts(new Account.Id(1001), SHA2)));
applyDelta(
c,
Delta.create(
c.getId(), metaId(SHA1),
drafts(new Account.Id(1001), SHA2)));
assertThat(c.getNoteDbState()).isEqualTo(
SHA1.name() + ",1001=" + SHA2.name());
applyDelta(c, Delta.create(c.getId(), noMetaId(),
drafts(new Account.Id(2003), SHA3)));
applyDelta(
c,
Delta.create(
c.getId(), noMetaId(),
drafts(new Account.Id(2003), SHA3)));
assertThat(c.getNoteDbState()).isEqualTo(
SHA1.name() + ",1001=" + SHA2.name() + ",2003=" + SHA3.name());
applyDelta(c, Delta.create(c.getId(), noMetaId(),
drafts(new Account.Id(2003), zeroId())));
applyDelta(
c,
Delta.create(
c.getId(), noMetaId(),
drafts(new Account.Id(2003), zeroId())));
assertThat(c.getNoteDbState()).isEqualTo(
SHA1.name() + ",1001=" + SHA2.name());
applyDelta(c, Delta.create(c.getId(), metaId(SHA3), noDrafts()));
applyDelta(
c, Delta.create(c.getId(), metaId(SHA3), noDrafts()));
assertThat(c.getNoteDbState()).isEqualTo(
SHA3.name() + ",1001=" + SHA2.name());
}
@Test
public void applyDeltaToReadOnly() throws Exception {
Timestamp ts = nowTs();
Change c = newChange();
NoteDbChangeState state = new NoteDbChangeState(c.getId(),
REVIEW_DB,
Optional.of(RefState.create(SHA1, ImmutableMap.of())),
Optional.of(new Timestamp(ts.getTime() + 10000)));
c.setNoteDbState(state.toString());
Delta delta = Delta.create(c.getId(), metaId(SHA2), noDrafts());
applyDelta(c, delta);
assertThat(NoteDbChangeState.parse(c)).isEqualTo(
new NoteDbChangeState(
state.getChangeId(),
state.getPrimaryStorage(),
Optional.of(RefState.create(SHA2, ImmutableMap.of())),
state.getReadOnlyUntil()));
}
@Test
public void parseNoteDbPrimary() {
NoteDbChangeState state = parse(new Change.Id(1), "N");
assertThat(state.getPrimaryStorage()).isEqualTo(NOTE_DB);
assertThat(state.getRefState().isPresent()).isFalse();
assertThat(state.getReadOnlyUntil().isPresent()).isFalse();
}
@Test(expected = IllegalArgumentException.class)
@@ -151,7 +225,7 @@ public class NoteDbChangeStateTest extends GerritBaseTests {
}
@Test
public void applyDeltaToNoteDbPrimaryIsNoOp() {
public void applyDeltaToNoteDbPrimaryIsNoOp() throws Exception {
Change c = newChange();
c.setNoteDbState("N");
applyDelta(c, Delta.create(c.getId(), metaId(SHA1),