opendev/gerrit
Code Issues Proposed changes

AbstractChangeNotes: Never open repo when NoteDb is off

Browse Source 
This line has seen some action before in I1867f633, and before that in
I265ef862. Rereading those commit messages, I see no reason why we
should be opening the repo and possibly triggering auto-rebuilding when
NoteDb writes and reads are both disabled.

This was causing NoSuchChangeExceptions in the case where the Change
contained a noteDbState field but the ref was absent. That logic is also
questionable, for reasons now mentioned in a TODO. But really we should
never be reaching the openHandle method when NoteDb is completely off.

Add a regression test that would have caught this.

Change-Id: If0970e1cf61e1d98ccbb3ce27549186f5771466a
This commit is contained in:
Dave Borowitz 2017-12-20 08:14:27 -05:00 committed by David Pursehouse
parent 3c7f7e67b5
commit c893c02b32
3 changed files with 29 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -1275,6 +1275,28 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
assertThat(newPs3.getCreatedOn()).isGreaterThan(ps1.getCreatedOn());
}
@Test
public void ignoreNoteDbStateWithNoCorrespondingRefWhenWritesAndReadsDisabled() throws Exception {
PushOneCommit.Result r = createChange();
Change.Id id = r.getChange().getId();
ReviewDb db = getUnwrappedDb();
Change c = db.changes().get(id);
c.setNoteDbState("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
db.changes().update(Collections.singleton(c));
c = db.changes().get(id);
String refName = RefNames.changeMetaRef(id);
assertThat(getMetaRef(project, refName)).isNull();
ChangeNotes notes = notesFactory.create(dbProvider.get(), project, id);
assertThat(notes.getChange().getRowVersion()).isEqualTo(c.getRowVersion());
notes = notesFactory.createChecked(dbProvider.get(), project, id);
assertThat(notes.getChange().getRowVersion()).isEqualTo(c.getRowVersion());
assertThat(getMetaRef(project, refName)).isNull();
}
private void assertChangesReadOnly(RestApiException e) throws Exception {
Throwable cause = e.getCause();
assertThat(cause).isInstanceOf(UpdateException.class);

5
gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
View File

@ -144,7 +144,10 @@ public abstract class AbstractChangeNotes<T> {
throw new OrmException("NoteDb is required to read change " + changeId);
}
boolean readOrWrite = read || args.migration.writeChanges();
if (!readOrWrite && !autoRebuild) {
if (!readOrWrite) {
// Don't even open the repo if we neither write to nor read from NoteDb. It's possible that
// there is some garbage in the noteDbState field and/or the repo, but at this point NoteDb is
// completely off so it's none of our business.
loadDefaults();
return self();
}

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

@ -604,6 +604,9 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
if (state == null) {
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?
throw new NoSuchChangeException(getChangeId());
}
}