opendev/gerrit
Code Issues Proposed changes

Fix auto-rebuilding of changes with missing NoteDb refs

Browse Source 
Change-Id: I2066da896df694554885cdbddb3bd6949c7d86ec
This commit is contained in:
Dave Borowitz 2017-12-22 08:51:37 -05:00
parent 05c99c5d83
commit 3d58cbeff4
2 changed files with 49 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

36
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
View File

@ -1313,6 +1313,36 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
assertThat(getMetaRef(project, refName)).isNull();
}
@Test
public void autoRebuildMissingRefWriteOnly() throws Exception {
setNotesMigration(true, false);
testAutoRebuildMissingRef();
}
@Test
public void autoRebuildMissingRefReadWrite() throws Exception {
setNotesMigration(true, true);
testAutoRebuildMissingRef();
}
private void testAutoRebuildMissingRef() throws Exception {
PushOneCommit.Result r = createChange();
Change.Id id = r.getChange().getId();
assertChangeUpToDate(true, id);
notesFactory.createChecked(db, project, id);
try (Repository repo = repoManager.openRepository(project)) {
RefUpdate ru = repo.updateRef(RefNames.changeMetaRef(id));
ru.setForceUpdate(true);
assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED);
}
assertChangeUpToDate(false, id);
notesFactory.createChecked(db, project, id);
assertChangeUpToDate(true, id);
}
private void assertChangesReadOnly(RestApiException e) throws Exception {
Throwable cause = e.getCause();
assertThat(cause).isInstanceOf(UpdateException.class);
@ -1336,8 +1366,10 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
Change c = getUnwrappedDb().changes().get(id);
assertThat(c).isNotNull();
assertThat(c.getNoteDbState()).isNotNull();
assertThat(NoteDbChangeState.parse(c).isChangeUpToDate(new RepoRefCache(repo)))
.isEqualTo(expected);
NoteDbChangeState state = NoteDbChangeState.parse(c);
assertThat(state).isNotNull();
assertThat(state.getPrimaryStorage()).isEqualTo(PrimaryStorage.REVIEW_DB);
assertThat(state.isChangeUpToDate(new RepoRefCache(repo))).isEqualTo(expected);
}
}

19
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
View File

@ -732,16 +732,27 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
protected LoadHandle openHandle(Repository repo) throws NoSuchChangeException, IOException {
if (autoRebuild) {
NoteDbChangeState state = NoteDbChangeState.parse(change);
if (args.migration.disableChangeReviewDb()) {
checkState(
state != null,
"shouldn't have null NoteDbChangeState when ReviewDb disabled: %s",
change);
}
ObjectId id = readRef(repo);
if (id == null) {
// Meta ref doesn't exist in NoteDb.
if (state == null) {
// Either ReviewDb change is being newly created, or it exists in ReviewDb but has not yet
// been rebuilt for the first time, e.g. because we just turned on write-only mode. In
// both cases, we don't want to auto-rebuild, just proceed with an empty ChangeNotes.
return super.openHandle(repo, id);
} else if (shouldExist) {
// TODO(dborowitz): This means we have a state recorded in noteDbState but the ref doesn't
// exist for whatever reason. Doesn't this mean we should trigger an auto-rebuild, rather
// than throwing?
} else if (shouldExist && state.getPrimaryStorage() == PrimaryStorage.NOTE_DB) {
throw new NoSuchChangeException(getChangeId());
}
// ReviewDb claims NoteDb state exists, but meta ref isn't present: fall through and
// auto-rebuild if necessary.
}
RefCache refs = this.refs != null ? this.refs : new RepoRefCache(repo);
if (!NoteDbChangeState.isChangeUpToDate(state, refs, getChangeId())) {