From 51e45017af6a370ce4f6483a600b78e3e5b8b09f Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 22 Jul 2016 16:24:17 -0400 Subject: [PATCH] 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 --- .../gerrit/server/notedb/ChangeNotes.java | 3 ++- .../server/notedb/ChangeRebuilderImpl.java | 27 +++++++++++++++++-- .../server/notedb/DraftCommentNotes.java | 3 ++- 3 files changed, 29 insertions(+), 4 deletions(-) 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 cdaf08c1bc..627fe6634a 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 @@ public class ChangeNotes extends AbstractChangeNotes { // // 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 8130913f79..30c6d10787 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 @@ 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() { @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; } 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 684e4f6a6e..08195e41d6 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 @@ public class DraftCommentNotes extends AbstractChangeNotes { 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(