ChangeRebuilderImpl: Don't always drop updates when losing race
Inside the AtomicUpdate where we check if the NoteDbChangeState has
changed, an unequal state comparison does not necessarily mean that
another thread has made the *same* update as this thread is trying to
make. It might have been the same, which is ok to drop, but it might
also be different. In the latter case, we were dropping the change
without even attempting to execute the NoteDbUpdateManager, and
returning to the caller as if the update was successful. The caller
would then fail with a MissingObjectException because the object in
question was never flushed.
Distinguish these two cases with two different OrmRuntimeException.
The latter case gets rethrown as an OrmException to indicate to the
caller that they should build an in-memory RevWalk off of the staged
results instead of trying to read from the repo.
Change-Id: I0510db01d2ef48b5cfa06898392a6c9e4b2136bb
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
index cdaf08c..627fe66 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -584,7 +584,8 @@
//
// Parse notes from the staged result so we can return something useful
// to the caller instead of throwing.
- log.debug("Rebuilding change {} failed", getChangeId());
+ log.debug("Rebuilding change {} failed: {}",
+ getChangeId(), e.getMessage());
args.metrics.autoRebuildFailureCount.increment(CHANGES);
rebuildResult = checkNotNull(r);
checkNotNull(r.newState());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java
index 8130913..30c6d10 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java
@@ -176,6 +176,16 @@
}
}
+ private static class ConflictingUpdateException extends OrmRuntimeException {
+ private static final long serialVersionUID = 1L;
+
+ ConflictingUpdateException(Change change, String expectedNoteDbState) {
+ super(String.format(
+ "Expected change %s to have noteDbState %s but was %s",
+ change.getId(), expectedNoteDbState, change.getNoteDbState()));
+ }
+ }
+
@Override
public Result rebuild(NoteDbUpdateManager manager,
ChangeBundle bundle) throws NoSuchChangeException, IOException,
@@ -217,8 +227,13 @@
db.changes().atomicUpdate(changeId, new AtomicUpdate<Change>() {
@Override
public Change update(Change change) {
- if (!Objects.equals(oldNoteDbState, change.getNoteDbState())) {
+ String currNoteDbState = change.getNoteDbState();
+ if (Objects.equals(currNoteDbState, newNoteDbState)) {
+ // Another thread completed the same rebuild we were about to.
throw new AbortUpdateException();
+ } else if (!Objects.equals(oldNoteDbState, currNoteDbState)) {
+ // Another thread updated the state to something else.
+ throw new ConflictingUpdateException(change, oldNoteDbState);
}
change.setNoteDbState(newNoteDbState);
return change;
@@ -233,7 +248,15 @@
throw new OrmException(NoteDbUpdateManager.CHANGES_READ_ONLY);
}
} catch (AbortUpdateException e) {
- // Drop this rebuild; another thread completed it.
+ // Drop this rebuild; another thread completed it. It's ok to not execute
+ // the update in this case, since the object referenced in the Result was
+ // flushed to the repo by whatever thread won the race.
+ } catch (ConflictingUpdateException e) {
+ // Rethrow as an OrmException so the caller knows to use staged results.
+ // Strictly speaking they are not completely up to date, but result we
+ // send to the caller is the same as if this rebuild had executed before
+ // the other thread.
+ throw new OrmException(e.getMessage());
}
return r;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
index 684e4f6..08195e4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
@@ -206,7 +206,8 @@
repo.scanForRepoChanges();
} catch (OrmException | IOException e) {
// See ChangeNotes#rebuildAndOpen.
- log.debug("Rebuilding change {} via drafts failed", getChangeId());
+ log.debug("Rebuilding change {} via drafts failed: {}",
+ getChangeId(), e.getMessage());
args.metrics.autoRebuildFailureCount.increment(CHANGES);
checkNotNull(r.staged());
return LoadHandle.create(
