opendev/gerrit
Code Issues Proposed changes

ChangeRebuilderImpl: Don't always drop updates when losing race

Browse Source 
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
This commit is contained in:
Dave Borowitz
2016-07-22 16:24:17 -04:00
parent 9448ed4c81
commit 51e45017af
3 changed files with 29 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -584,7 +584,8 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
//
// 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());

27
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java
View File

@@ -176,6 +176,16 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
}
}
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 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
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 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
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;
}

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

@@ -206,7 +206,8 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
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(