opendev/gerrit
Code Issues Proposed changes

NoteDb: Avoid writing partial change graphs

Browse Source 
If we naively turn on notedb.writeChanges without previously running
RebuildNoteDb to backfill existing changes, a single ChangeUpdate may
end up writing a partial note graph, for example containing a single
commit with only a "Topic" field and no other information. Such a
partial graph will almost certainly fail to parse; better to avoid
writing them out in the first place.

Teach NoteDbUpdateManager to skip writing updates unless the first
update in the list has a special allowWriteToNewRef. This means
writing a change will automatically "turn on" on a per-change basis as
soon as a backfill is completed, whether in a running server or in
between restarts.

There are three cases where we want to allow writing updates to new
refs:
 - Any ChangeDraftUpdate, since having a "partial" draft notes ref
   doesn't cause parsing to fail, and executing the same update
   multiple times is idempotent with respect to the note data.
 - A ChangeUpdate that creates a change for the first time, which is
   by definition not a partial graph. This is handled in BatchUpdate,
   which already knows which ChangeUpdates correspond to new changes.
 - In ChangeRebuilder, which again is guaranteed to write a complete
   graph.

Change-Id: Ic8c6c28e68aef22be7569d2b0cb409eae22e3034
This commit is contained in:
Dave Borowitz
2016-03-24 13:36:15 -04:00
parent f449ad4ce7
commit d2c9c2ed93
8 changed files with 100 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -51,4 +51,43 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
r = amendChange(r.getChangeId());
checker.rebuildAndCheckChanges(id);
}
@Test
public void noWriteToNewRef() throws Exception {
PushOneCommit.Result r = createChange();
Change.Id id = r.getPatchSetId().getParentKey();
checker.assertNoChangeRef(project, id);
notesMigration.setWriteChanges(true);
gApi.changes().id(id.get()).topic(name("a-topic"));
// First write doesn't create the ref, but rebuilding works.
checker.assertNoChangeRef(project, id);
checker.rebuildAndCheckChanges(id);
// Now that there is a ref, writes are "turned on" for this change, and
// NoteDb stays up to date without explicit rebuilding.
gApi.changes().id(id.get()).topic(name("new-topic"));
checker.checkChanges(id);
}
@Test
public void writeToNewRefForNewChange() throws Exception {
PushOneCommit.Result r1 = createChange();
Change.Id id1 = r1.getPatchSetId().getParentKey();
notesMigration.setWriteChanges(true);
gApi.changes().id(id1.get()).topic(name("a-topic"));
PushOneCommit.Result r2 = createChange();
Change.Id id2 = r2.getPatchSetId().getParentKey();
// Second change was created after NoteDb writes were turned on, so it was
// allowed to write to a new ref.
checker.checkChanges(id2);
// First change was created before NoteDb writes were turned on, so its meta
// ref doesn't exist until a manual rebuild.
checker.assertNoChangeRef(project, id1);
checker.rebuildAndCheckChanges(id1);
}
}

3
gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java
View File

@@ -196,6 +196,9 @@ public class BatchUpdate implements AutoCloseable {
ChangeUpdate u = updates.get(psId);
if (u == null) {
u = changeUpdateFactory.create(ctl, when);
if (newChanges.containsKey(ctl.getId())) {
u.setAllowWriteToNewRef(true);
}
u.setPatchSetId(psId);
updates.put(psId, u);
}

4
gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java
View File

@@ -183,6 +183,10 @@ public abstract class AbstractChangeUpdate {
return result;
}
public boolean allowWriteToNewRef() {
return true;
}
private static ObjectId emptyTree(ObjectInserter ins) throws IOException {
return ins.insert(Constants.OBJ_TREE, new byte[] {});
}

1
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java
View File

@@ -235,6 +235,7 @@ public class ChangeRebuilder {
ChangeUpdate update = updateFactory.create(
controlFactory.controlFor(db, change, events.getUser(db)),
events.getWhen());
update.setAllowWriteToNewRef(true);
update.setPatchSetId(events.getPatchSetId());
for (Event e : events) {
e.apply(update);

10
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
View File

@@ -121,6 +121,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
private PatchSetState psState;
private Iterable<String> groups;
private String pushCert;
private boolean isAllowWriteToNewtRef;
private ChangeDraftUpdate draftUpdate;
@@ -606,6 +607,15 @@ public class ChangeUpdate extends AbstractChangeUpdate {
return draftUpdate;
}
public void setAllowWriteToNewRef(boolean allow) {
isAllowWriteToNewtRef = allow;
}
@Override
public boolean allowWriteToNewRef() {
return isAllowWriteToNewtRef;
}
private static StringBuilder addFooter(StringBuilder sb, FooterKey footer) {
return sb.append(footer.getName()).append(": ");
}

28
gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
View File

@@ -40,6 +40,8 @@ import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand;
import java.io.IOException;
import java.util.Collection;
import java.util.Map;
public class NoteDbUpdateManager {
public interface Factory {
@@ -248,15 +250,23 @@ public class NoteDbUpdateManager {
}
}
private static void addUpdates(
ListMultimap<String, ? extends AbstractChangeUpdate> updates, OpenRepo or)
private static <U extends AbstractChangeUpdate> void addUpdates(
ListMultimap<String, U> all, OpenRepo or)
throws OrmException, IOException {
for (String refName : updates.keySet()) {
for (Map.Entry<String, Collection<U>> e : all.asMap().entrySet()) {
String refName = e.getKey();
Collection<U> updates = e.getValue();
ObjectId old = firstNonNull(
or.cmds.getObjectId(or.repo, refName), ObjectId.zeroId());
ObjectId curr = old;
// Only actually write to the ref if one of the updates explicitly allows
// us to do so, i.e. it is known to represent a new change. This avoids
// writing partial change meta if the change hasn't been backfilled yet.
if (!allowWrite(updates, old)) {
continue;
}
for (AbstractChangeUpdate u : updates.get(refName)) {
ObjectId curr = old;
for (U u : updates) {
ObjectId next = u.apply(or.rw, or.ins, curr);
if (next == null) {
continue;
@@ -268,4 +278,12 @@ public class NoteDbUpdateManager {
}
}
}
private static <U extends AbstractChangeUpdate> boolean allowWrite(
Collection<U> updates, ObjectId old) {
if (!old.equals(ObjectId.zeroId())) {
return true;
}
return updates.iterator().next().allowWriteToNewRef();
}
}

4
gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
View File

@@ -204,7 +204,9 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests {
protected ChangeUpdate newUpdate(Change c, CurrentUser user)
throws Exception {
return TestChanges.newUpdate(injector, c, user);
ChangeUpdate update = TestChanges.newUpdate(injector, c, user);
update.setAllowWriteToNewRef(true);
return update;
}
protected ChangeNotes newNotes(Change c) throws OrmException {

17
gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbChecker.java
View File

@@ -14,13 +14,18 @@
package com.google.gerrit.testutil;
import static com.google.common.truth.Truth.assertThat;
import com.google.common.base.Joiner;
import com.google.common.collect.Iterables;
import com.google.gerrit.reviewdb.client.Change;
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.server.PatchLineCommentsUtil;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeBundle;
import com.google.gerrit.server.notedb.ChangeNoteUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeRebuilder;
import com.google.gerrit.server.schema.DisabledChangesReviewDbWrapper;
@@ -29,6 +34,7 @@ import com.google.inject.Provider;
import com.google.inject.Singleton;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.Repository;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -41,6 +47,7 @@ public class NoteDbChecker {
static final Logger log = LoggerFactory.getLogger(NoteDbChecker.class);
private final Provider<ReviewDb> dbProvider;
private final GitRepositoryManager repoManager;
private final TestNotesMigration notesMigration;
private final ChangeNotes.Factory notesFactory;
private final ChangeRebuilder changeRebuilder;
@@ -48,11 +55,13 @@ public class NoteDbChecker {
@Inject
NoteDbChecker(Provider<ReviewDb> dbProvider,
GitRepositoryManager repoManager,
TestNotesMigration notesMigration,
ChangeNotes.Factory notesFactory,
ChangeRebuilder changeRebuilder,
PatchLineCommentsUtil plcUtil) {
this.dbProvider = dbProvider;
this.repoManager = repoManager;
this.notesMigration = notesMigration;
this.notesFactory = notesFactory;
this.changeRebuilder = changeRebuilder;
@@ -106,6 +115,14 @@ public class NoteDbChecker {
checkActual(readExpected(changeIds), new ArrayList<String>());
}
public void assertNoChangeRef(Project.NameKey project, Change.Id changeId)
throws Exception {
try (Repository repo = repoManager.openMetadataRepository(project)) {
assertThat(repo.exactRef(ChangeNoteUtil.changeRefName(changeId)))
.isNull();
}
}
private List<ChangeBundle> readExpected(Iterable<Change.Id> changeIds)
throws Exception {
ReviewDb db = unwrapDb();