Merge changes from topic 'notedb-primary'

* changes:
  NoteDbChangeState: Add enum to indicate change's primary storage
  NoteDbChangeState: Refactor to move ref state into nested class
This commit is contained in:
Dave Borowitz
2016-11-03 20:58:40 +00:00
committed by Gerrit Code Review
6 changed files with 255 additions and 80 deletions

View File

@@ -69,6 +69,7 @@ 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;
import com.google.gerrit.server.notedb.NoteDbUpdateManager;
import com.google.gerrit.server.notedb.TestChangeRebuilderWrapper;
import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder.NoPatchSetsException;
@@ -100,6 +101,7 @@ import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.TimeUnit;
public class ChangeRebuilderIT extends AbstractDaemonTest {
@@ -593,11 +595,15 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
ReviewDb db = getUnwrappedDb();
Change c = db.changes().get(id);
// Leave change meta ID alone so DraftCommentNotes does the rebuild.
ObjectId badSha =
ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
NoteDbChangeState bogusState = new NoteDbChangeState(
id, NoteDbChangeState.parse(c).getChangeMetaId(),
ImmutableMap.<Account.Id, ObjectId>of(
user.getId(),
ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")));
id,
PrimaryStorage.REVIEW_DB,
Optional.of(
NoteDbChangeState.RefState.create(
NoteDbChangeState.parse(c).getChangeMetaId(),
ImmutableMap.of(user.getId(), badSha))));
c.setNoteDbState(bogusState.toString());
db.changes().update(Collections.singleton(c));

View File

@@ -44,7 +44,6 @@ import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.reviewdb.server.ReviewDbWrapper;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.GerritPersonIdent;
@@ -1014,7 +1013,7 @@ public class BatchUpdate implements AutoCloseable {
RevWalk rw, Change.Id id) throws Exception {
Change c = newChanges.get(id);
if (c == null) {
c = ReviewDbUtil.unwrapDb(db).changes().get(id);
c = ChangeNotes.readOneReviewDbChange(db, id);
}
// Pass in preloaded change to controlFor, to avoid:
// - reading from a db that does not belong to this update

View File

@@ -52,6 +52,7 @@ import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.ReviewerStatusUpdate;
import com.google.gerrit.server.git.RefCache;
import com.google.gerrit.server.git.RepoRefCache;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
@@ -95,6 +96,20 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
+ String.format(fmt, args));
}
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;
}
@Singleton
public static class Factory {
private final Args args;
@@ -118,7 +133,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
public ChangeNotes createChecked(ReviewDb db, Project.NameKey project,
Change.Id changeId) throws OrmException, NoSuchChangeException {
Change change = ReviewDbUtil.unwrapDb(db).changes().get(changeId);
Change change = readOneReviewDbChange(db, changeId);
if (change == null || !change.getProject().equals(project)) {
throw new NoSuchChangeException(changeId);
}
@@ -142,7 +157,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
private Change loadChangeFromDb(ReviewDb db, Project.NameKey project,
Change.Id changeId) throws OrmException {
Change change = ReviewDbUtil.unwrapDb(db).changes().get(changeId);
Change change = readOneReviewDbChange(db, changeId);
checkArgument(project != null, "project is required");
checkNotNull(change,
"change %s not found in ReviewDb", changeId);
@@ -261,6 +276,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
}
} else {
for (Change change : ReviewDbUtil.unwrapDb(db).changes().all()) {
checkNoteDbState(change);
ChangeNotes notes = createFromChangeOnlyWhenNoteDbDisabled(change);
if (predicate.test(notes)) {
m.put(change.getProject(), notes);
@@ -297,9 +313,8 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
Project.NameKey project) throws OrmException, IOException {
Set<Change.Id> ids = scan(repo);
List<ChangeNotes> changeNotes = new ArrayList<>(ids.size());
db = ReviewDbUtil.unwrapDb(db);
for (Change.Id id : ids) {
Change change = db.changes().get(id);
Change change = readOneReviewDbChange(db, id);
if (change == null) {
log.warn("skipping change {} found in project {} " +
"but not in ReviewDb",

View File

@@ -16,30 +16,30 @@ package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef;
import static com.google.gerrit.reviewdb.client.RefNames.refsDraftComments;
import static java.util.Comparator.comparing;
import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.NOTE_DB;
import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB;
import static org.eclipse.jgit.lib.ObjectId.zeroId;
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Predicates;
import com.google.common.base.Splitter;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.gerrit.common.Nullable;
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.Collections;
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
@@ -48,13 +48,35 @@ import java.util.Optional;
* Stored serialized in the {@code Change#noteDbState} field, and used to
* determine whether the state in NoteDb is out of date.
* <p>
* Serialized in the form:
* <pre>
* [meta-sha],[account1]=[drafts-sha],[account2]=[drafts-sha]...
* </pre>
* Serialized in one of the forms:
* <ul>
* <li>[meta-sha],[account1]=[drafts-sha],[account2]=[drafts-sha]...
* <li>R[meta-sha],[account1]=[drafts-sha],[account2]=[drafts-sha]...
* <li>N
* </ul>
*
* in numeric account ID order, with hex SHA-1s for human readability.
*/
public class NoteDbChangeState {
public static final String NOTE_DB_PRIMARY_STATE = "N";
public enum PrimaryStorage {
REVIEW_DB('R', true),
NOTE_DB('N', false);
private final char code;
private final boolean writeToReviewDb;
private PrimaryStorage(char code, boolean writeToReviewDb) {
this.code = code;
this.writeToReviewDb = writeToReviewDb;
}
public boolean writeToReviewDb() {
return writeToReviewDb;
}
}
@AutoValue
public abstract static class Delta {
static Delta create(Change.Id changeId, Optional<ObjectId> newChangeMetaId,
@@ -73,31 +95,89 @@ public class NoteDbChangeState {
abstract ImmutableMap<Account.Id, ObjectId> newDraftIds();
}
@AutoValue
public abstract static class RefState {
@VisibleForTesting
public static RefState create(ObjectId changeMetaId,
Map<Account.Id, ObjectId> draftIds) {
return new AutoValue_NoteDbChangeState_RefState(
changeMetaId.copy(),
ImmutableMap.copyOf(
Maps.filterValues(draftIds, id -> !zeroId().equals(id))));
}
private static Optional<RefState> parse(Change.Id changeId,
List<String> parts) {
checkArgument(!parts.isEmpty(),
"missing state string for change %s", changeId);
ObjectId changeMetaId = ObjectId.fromString(parts.get(0));
Map<Account.Id, ObjectId> draftIds =
Maps.newHashMapWithExpectedSize(parts.size() - 1);
Splitter s = Splitter.on('=');
for (int i = 1; i < parts.size(); i++) {
String p = parts.get(i);
List<String> draftParts = s.splitToList(p);
checkArgument(draftParts.size() == 2,
"invalid draft state part for change %s: %s", changeId, p);
draftIds.put(Account.Id.parse(draftParts.get(0)),
ObjectId.fromString(draftParts.get(1)));
}
return Optional.of(create(changeMetaId, draftIds));
}
abstract ObjectId changeMetaId();
abstract ImmutableMap<Account.Id, ObjectId> draftIds();
@Override
public String toString() {
return appendTo(new StringBuilder()).toString();
}
StringBuilder appendTo(StringBuilder sb) {
sb.append(changeMetaId().name());
for (Account.Id id : ReviewDbUtil.intKeyOrdering()
.sortedCopy(draftIds().keySet())) {
sb.append(',')
.append(id.get())
.append('=')
.append(draftIds().get(id).name());
}
return sb;
}
}
public static NoteDbChangeState parse(Change c) {
return parse(c.getId(), c.getNoteDbState());
return c != null ? parse(c.getId(), c.getNoteDbState()) : null;
}
@VisibleForTesting
public static NoteDbChangeState parse(Change.Id id, String str) {
if (str == null) {
if (Strings.isNullOrEmpty(str)) {
// Return null rather than Optional as this is what goes in the field in
// ReviewDb.
return null;
}
List<String> parts = Splitter.on(',').splitToList(str);
checkArgument(!parts.isEmpty(),
"invalid state string for change %s: %s", id, str);
ObjectId changeMetaId = ObjectId.fromString(parts.get(0));
Map<Account.Id, ObjectId> draftIds =
Maps.newHashMapWithExpectedSize(parts.size() - 1);
Splitter s = Splitter.on('=');
for (int i = 1; i < parts.size(); i++) {
String p = parts.get(i);
List<String> draftParts = s.splitToList(p);
checkArgument(draftParts.size() == 2,
"invalid draft state part for change %s: %s", id, p);
draftIds.put(Account.Id.parse(draftParts.get(0)),
ObjectId.fromString(draftParts.get(1)));
// Only valid NOTE_DB state is "N".
String first = parts.get(0);
if (parts.size() == 1 && first.charAt(0) == NOTE_DB.code) {
return new NoteDbChangeState(id, NOTE_DB, Optional.empty());
}
return new NoteDbChangeState(id, changeMetaId, draftIds);
// Otherwise it must be REVIEW_DB, either "R,<RefState>" or just
// "<RefState>". Allow length > 0 for forward compatibility.
if (first.length() > 0) {
Optional<RefState> refState;
if (first.charAt(0) == REVIEW_DB.code) {
refState = RefState.parse(id, parts.subList(1, parts.size()));
} else {
refState = RefState.parse(id, parts);
}
return new NoteDbChangeState(id, REVIEW_DB, refState);
}
throw new IllegalArgumentException(
"invalid state string for change " + id + ": " + str);
}
public static NoteDbChangeState applyDelta(Change change, Delta delta) {
@@ -112,6 +192,10 @@ public class NoteDbChangeState {
return null;
}
NoteDbChangeState oldState = parse(change.getId(), oldStr);
if (oldState != null && oldState.getPrimaryStorage() == NOTE_DB) {
// NOTE_DB state doesn't include RefState, so applying a delta is a no-op.
return oldState;
}
ObjectId changeMetaId;
if (delta.newChangeMetaId().isPresent()) {
@@ -121,12 +205,12 @@ public class NoteDbChangeState {
return null;
}
} else {
changeMetaId = oldState.changeMetaId;
changeMetaId = oldState.getChangeMetaId();
}
Map<Account.Id, ObjectId> draftIds = new HashMap<>();
if (oldState != null) {
draftIds.putAll(oldState.draftIds);
draftIds.putAll(oldState.getDraftIds());
}
for (Map.Entry<Account.Id, ObjectId> e : delta.newDraftIds().entrySet()) {
if (e.getValue().equals(ObjectId.zeroId())) {
@@ -137,7 +221,11 @@ public class NoteDbChangeState {
}
NoteDbChangeState state = new NoteDbChangeState(
change.getId(), changeMetaId, draftIds);
change.getId(),
oldState != null
? oldState.getPrimaryStorage()
: REVIEW_DB,
Optional.of(RefState.create(changeMetaId, draftIds)));
change.setNoteDbState(state.toString());
return state;
}
@@ -160,38 +248,47 @@ public class NoteDbChangeState {
return state.areDraftsUpToDate(draftsRepoRefs, accountId);
}
public static String toString(ObjectId changeMetaId,
Map<Account.Id, ObjectId> draftIds) {
List<Account.Id> accountIds = Lists.newArrayList(draftIds.keySet());
Collections.sort(accountIds, comparing(Account.Id::get));
StringBuilder sb = new StringBuilder(changeMetaId.name());
for (Account.Id id : accountIds) {
sb.append(',')
.append(id.get())
.append('=')
.append(draftIds.get(id).name());
private final Change.Id changeId;
private final PrimaryStorage primaryStorage;
private final Optional<RefState> refState;
public NoteDbChangeState(
Change.Id changeId,
PrimaryStorage primaryStorage,
Optional<RefState> refState) {
this.changeId = checkNotNull(changeId);
this.primaryStorage = checkNotNull(primaryStorage);
this.refState = refState;
switch (primaryStorage) {
case REVIEW_DB:
checkArgument(
refState.isPresent(),
"expected RefState for change %s with primary storage %s",
changeId, primaryStorage);
break;
case NOTE_DB:
checkArgument(
!refState.isPresent(),
"expected no RefState for change %s with primary storage %s",
changeId, primaryStorage);
break;
default:
throw new IllegalStateException(
"invalid PrimaryStorage: " + primaryStorage);
}
return sb.toString();
}
private final Change.Id changeId;
private final ObjectId changeMetaId;
private final ImmutableMap<Account.Id, ObjectId> draftIds;
public NoteDbChangeState(Change.Id changeId, ObjectId changeMetaId,
Map<Account.Id, ObjectId> draftIds) {
this.changeId = checkNotNull(changeId);
this.changeMetaId = checkNotNull(changeMetaId);
this.draftIds = ImmutableMap.copyOf(Maps.filterValues(
draftIds, Predicates.not(Predicates.equalTo(ObjectId.zeroId()))));
public PrimaryStorage getPrimaryStorage() {
return primaryStorage;
}
public boolean isChangeUpToDate(RefCache changeRepoRefs) throws IOException {
Optional<ObjectId> id = changeRepoRefs.get(changeMetaRef(changeId));
if (!id.isPresent()) {
return changeMetaId.equals(ObjectId.zeroId());
return getChangeMetaId().equals(ObjectId.zeroId());
}
return id.get().equals(changeMetaId);
return id.get().equals(getChangeMetaId());
}
public boolean areDraftsUpToDate(RefCache draftsRepoRefs, Account.Id accountId)
@@ -199,9 +296,9 @@ public class NoteDbChangeState {
Optional<ObjectId> id =
draftsRepoRefs.get(refsDraftComments(changeId, accountId));
if (!id.isPresent()) {
return !draftIds.containsKey(accountId);
return !getDraftIds().containsKey(accountId);
}
return id.get().equals(draftIds.get(accountId));
return id.get().equals(getDraftIds().get(accountId));
}
public boolean isUpToDate(RefCache changeRepoRefs, RefCache draftsRepoRefs)
@@ -209,7 +306,7 @@ public class NoteDbChangeState {
if (!isChangeUpToDate(changeRepoRefs)) {
return false;
}
for (Account.Id accountId : draftIds.keySet()) {
for (Account.Id accountId : getDraftIds().keySet()) {
if (!areDraftsUpToDate(draftsRepoRefs, accountId)) {
return false;
}
@@ -224,16 +321,36 @@ public class NoteDbChangeState {
@VisibleForTesting
public ObjectId getChangeMetaId() {
return changeMetaId;
return refState().changeMetaId();
}
@VisibleForTesting
ImmutableMap<Account.Id, ObjectId> getDraftIds() {
return draftIds;
return refState().draftIds();
}
@VisibleForTesting
Optional<RefState> getRefState() {
return refState;
}
private RefState refState() {
checkState(refState.isPresent(),
"state for %s has no RefState: %s", changeId, this);
return refState.get();
}
@Override
public String toString() {
return toString(changeMetaId, draftIds);
switch (primaryStorage) {
case REVIEW_DB:
// Don't include enum field, just IDs (though parse would accept it).
return refState().toString();
case NOTE_DB:
return NOTE_DB_PRIMARY_STATE;
default:
throw new IllegalArgumentException(
"Unsupported PrimaryStorage: " + primaryStorage);
}
}
}

View File

@@ -54,6 +54,7 @@ import com.google.gerrit.server.notedb.ChangeBundle;
import com.google.gerrit.server.notedb.ChangeBundleReader;
import com.google.gerrit.server.notedb.ChangeDraftUpdate;
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.NoteDbUpdateManager;
@@ -184,7 +185,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
public NoteDbUpdateManager stage(ReviewDb db, Change.Id changeId)
throws NoSuchChangeException, IOException, OrmException {
db = ReviewDbUtil.unwrapDb(db);
Change change = db.changes().get(changeId);
Change change = ChangeNotes.readOneReviewDbChange(db, changeId);
if (change == null) {
throw new NoSuchChangeException(changeId);
}
@@ -200,7 +201,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
NoteDbUpdateManager manager) throws NoSuchChangeException, OrmException,
IOException {
db = ReviewDbUtil.unwrapDb(db);
Change change = db.changes().get(changeId);
Change change = ChangeNotes.readOneReviewDbChange(db, changeId);
if (change == null) {
throw new NoSuchChangeException(changeId);
}

View File

@@ -18,6 +18,8 @@ import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.truth.Truth.assertThat;
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;
import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB;
import static org.eclipse.jgit.lib.ObjectId.zeroId;
import com.google.common.collect.ImmutableMap;
@@ -48,30 +50,44 @@ public class NoteDbChangeStateTest {
ObjectId.fromString("badc0feebadc0feebadc0feebadc0feebadc0fee");
@Test
public void parseWithoutDrafts() {
public void parseReviewDbWithoutDrafts() {
NoteDbChangeState state = parse(new Change.Id(1), SHA1.name());
assertThat(state.getPrimaryStorage()).isEqualTo(REVIEW_DB);
assertThat(state.getChangeId()).isEqualTo(new Change.Id(1));
assertThat(state.getChangeMetaId()).isEqualTo(SHA1);
assertThat(state.getDraftIds()).isEmpty();
assertThat(state.toString()).isEqualTo(SHA1.name());
state = parse(new Change.Id(1), "R," + SHA1.name());
assertThat(state.getPrimaryStorage()).isEqualTo(REVIEW_DB);
assertThat(state.getChangeId()).isEqualTo(new Change.Id(1));
assertThat(state.getChangeMetaId()).isEqualTo(SHA1);
assertThat(state.getDraftIds()).isEmpty();
assertThat(state.toString()).isEqualTo(SHA1.name());
}
@Test
public void parseWithDrafts() {
NoteDbChangeState state = parse(
new Change.Id(1),
SHA1.name() + ",2003=" + SHA2.name() + ",1001=" + SHA3.name());
public void parseReviewDbWithDrafts() {
String str = SHA1.name() + ",2003=" + SHA2.name() + ",1001=" + SHA3.name();
String expected =
SHA1.name() + ",1001=" + SHA3.name() + ",2003=" + SHA2.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.getDraftIds()).containsExactly(
new Account.Id(1001), SHA3,
new Account.Id(2003), SHA2);
assertThat(state.toString()).isEqualTo(expected);
assertThat(state.toString()).isEqualTo(
SHA1.name() + ",1001=" + SHA3.name() + ",2003=" + SHA2.name());
state = parse(new Change.Id(1), "R," + str);
assertThat(state.getPrimaryStorage()).isEqualTo(REVIEW_DB);
assertThat(state.getChangeId()).isEqualTo(new Change.Id(1));
assertThat(state.getChangeMetaId()).isEqualTo(SHA1);
assertThat(state.getDraftIds()).containsExactly(
new Account.Id(1001), SHA3,
new Account.Id(2003), SHA2);
assertThat(state.toString()).isEqualTo(expected);
}
@Test
@@ -127,6 +143,27 @@ public class NoteDbChangeStateTest {
SHA3.name() + ",1001=" + SHA2.name());
}
@Test
public void parseNoteDbPrimary() {
NoteDbChangeState state = parse(new Change.Id(1), "N");
assertThat(state.getPrimaryStorage()).isEqualTo(NOTE_DB);
assertThat(state.getRefState().isPresent()).isFalse();
}
@Test(expected = IllegalArgumentException.class)
public void parseInvalidPrimaryStorage() {
parse(new Change.Id(1), "X");
}
@Test
public void applyDeltaToNoteDbPrimaryIsNoOp() {
Change c = newChange();
c.setNoteDbState("N");
applyDelta(c, Delta.create(c.getId(), metaId(SHA1),
drafts(new Account.Id(1001), SHA2)));
assertThat(c.getNoteDbState()).isEqualTo("N");
}
private static Change newChange() {
return TestChanges.newChange(
new Project.NameKey("project"), new Account.Id(12345));