Selectively ignore errors from NoteDbUpdateManager

As we know by now, NoteDb writes can fail for all sorts of reasons.
In the case of writes via BatchUpdate, from the user perspective we
don't actually care if the NoteDb update fails. If NoteDb reads are
enabled, we want the change to get rebuilt on the next read, but that
will happen regardless of whether or not BatchUpdate throws an error,
because we've already saved the data in the canonical location in
ReviewDb. Users prefer not to see errors, so just ignore them.

Another somewhat related issue is what to do about errors during the
auto-rebuild codepath during read. These have been cropping up
unpleasantly frequently due to the behavior of the change screen,
which sends a bunch of parallel requests all reading from the same
change entity. This is racy and tends to result in a noticeable
subset of requests getting LOCK_FAILURE and throwing. In this case,
the caller does actually care that the change got successfully rebuilt
after we detected it as stale--we can't just ignore the error. But it
doesn't actually matter that *this caller* was the one to rebuild the
change. We can just reread the NoteDbChangeState from ReviewDb and
compare it to the new value of NoteDb after an exception to see if it
got rebuilt behind our backs. Only go arm's-length here and retry
once; rebuilding in a loop seems risky.

Both of these cases are actually quite straightforward to fix; the
annoying thing is injecting a test ChangeRebuilder implementation
that's capable of doing the update behind the caller's back.

Change-Id: I0b80c97552cbdbfc6de44a785b41ad7ad8ab05b0

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

@@ -24,6 +24,7 @@ import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 import com.google.common.collect.ImmutableList;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.AcceptanceTestRequestScope;
+import com.google.gerrit.acceptance.GerritConfig;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.TestAccount;
 import com.google.gerrit.common.TimeUtil;
@@ -50,6 +51,7 @@ import com.google.gerrit.server.git.RepoRefCache;
 import com.google.gerrit.server.notedb.ChangeBundle;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.NoteDbChangeState;
+import com.google.gerrit.server.notedb.TestChangeRebuilderWrapper;
 import com.google.gerrit.server.schema.DisabledChangesReviewDbWrapper;
 import com.google.gerrit.testutil.NoteDbChecker;
 import com.google.gerrit.testutil.NoteDbMode;
@@ -92,6 +94,9 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
   @Inject
   private Provider<PostReview> postReview;
 
+  @Inject
+  private TestChangeRebuilderWrapper rebuilderWrapper;
+
   @Before
   public void setUp() {
     assume().that(NoteDbMode.readWrite()).isFalse();
@@ -352,6 +357,36 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     assertThat(actual.differencesFrom(expected)).isEmpty();
   }
 
+  @Test
+  @GerritConfig(name = "noteDb.testRebuilderWrapper", value = "true")
+  public void rebuildIgnoresErrorIfChangeIsUpToDateAfter() throws Exception {
+    setNotesMigration(true, true);
+
+    PushOneCommit.Result r = createChange();
+    Change.Id id = r.getPatchSetId().getParentKey();
+    assertChangeUpToDate(true, id);
+
+    // Make a ReviewDb change behind NoteDb's back and ensure it's detected.
+    setNotesMigration(false, false);
+    gApi.changes().id(id.get()).topic(name("a-topic"));
+    setInvalidNoteDbState(id);
+    assertChangeUpToDate(false, id);
+
+    // Force the next rebuild attempt to fail but also rebuild the change in the
+    // background.
+    rebuilderWrapper.stealNextUpdate();
+    setNotesMigration(true, true);
+    assertThat(gApi.changes().id(id.get()).info().topic)
+        .isEqualTo(name("a-topic"));
+    assertChangeUpToDate(true, id);
+
+    // Check that the bundles are equal.
+    ChangeBundle actual = ChangeBundle.fromNotes(
+        plcUtil, notesFactory.create(dbProvider.get(), project, id));
+    ChangeBundle expected = ChangeBundle.fromReviewDb(unwrapDb(), id);
+    assertThat(actual.differencesFrom(expected)).isEmpty();
+  }
+
   @Test
   public void rebuildAutomaticallyWhenDraftsOutOfDate() throws Exception {
     setNotesMigration(true, true);

gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/BatchProgramModule.java

@@ -42,6 +42,7 @@ import com.google.gerrit.server.config.CanonicalWebUrl;
 import com.google.gerrit.server.config.CanonicalWebUrlProvider;
 import com.google.gerrit.server.config.DisableReverseDnsLookup;
 import com.google.gerrit.server.config.DisableReverseDnsLookupProvider;
+import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.GitReceivePackGroups;
 import com.google.gerrit.server.config.GitUploadPackGroups;
 import com.google.gerrit.server.git.BatchUpdate;
@@ -65,6 +66,8 @@ import com.google.inject.Module;
 import com.google.inject.TypeLiteral;
 import com.google.inject.util.Providers;
 
+import org.eclipse.jgit.lib.Config;
+
 import java.util.Collections;
 import java.util.List;
 import java.util.Set;
@@ -77,10 +80,13 @@ import java.util.Set;
  * concurrently.
  */
 public class BatchProgramModule extends FactoryModule {
+  private final Config cfg;
   private final Module reviewDbModule;
 
   @Inject
-  BatchProgramModule(PerThreadReviewDbModule reviewDbModule) {
+  BatchProgramModule(@GerritServerConfig Config cfg,
+      PerThreadReviewDbModule reviewDbModule) {
+    this.cfg = cfg;
     this.reviewDbModule = reviewDbModule;
   }
 
@@ -126,7 +132,7 @@ public class BatchProgramModule extends FactoryModule {
     install(new BatchGitModule());
     install(new DefaultCacheFactory.Module());
     install(new GroupModule());
-    install(new NoteDbModule());
+    install(new NoteDbModule(cfg));
     install(new PrologModule());
     install(AccountByEmailCacheImpl.module());
     install(AccountCacheImpl.module());

gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java

@@ -154,6 +154,7 @@ import com.google.inject.TypeLiteral;
 import com.google.inject.internal.UniqueAnnotations;
 
 import org.apache.velocity.runtime.RuntimeInstance;
+import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.transport.PostReceiveHook;
 import org.eclipse.jgit.transport.PreUploadHook;
 
@@ -162,10 +163,14 @@ import java.util.List;
 
 /** Starts global state with standard dependencies. */
 public class GerritGlobalModule extends FactoryModule {
+  private final Config cfg;
   private final AuthModule authModule;
 
   @Inject
-  GerritGlobalModule(AuthModule authModule) {
+  GerritGlobalModule(
+      @GerritServerConfig Config cfg,
+      AuthModule authModule) {
+    this.cfg = cfg;
     this.authModule = authModule;
   }
 
@@ -198,7 +203,7 @@ public class GerritGlobalModule extends FactoryModule {
     install(new EmailModule());
     install(new GitModule());
     install(new GroupModule());
-    install(new NoteDbModule());
+    install(new NoteDbModule(cfg));
     install(new PrologModule());
     install(new SshAddressesModule());
     install(ThreadLocalRequestContext.module());

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

@@ -61,6 +61,8 @@ import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.ReceiveCommand;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.sql.Timestamp;
@@ -96,6 +98,8 @@ import java.util.TreeMap;
  * next phase.
  */
 public class BatchUpdate implements AutoCloseable {
+  private static final Logger log = LoggerFactory.getLogger(BatchUpdate.class);
+
   public interface Factory {
     BatchUpdate create(ReviewDb db, Project.NameKey project,
         CurrentUser user, Timestamp when);
@@ -595,14 +599,22 @@ public class BatchUpdate implements AutoCloseable {
           db.rollback();
         }
 
-        // Execute NoteDb updates after committing ReviewDb updates.
         if (notesMigration.writeChanges()) {
+          try {
             if (updateManager != null) {
+              // Execute NoteDb updates after committing ReviewDb updates.
               updateManager.execute();
             }
             if (ctx.deleted) {
               new ChangeDelete(plcUtil, getRepository(), ctx.getNotes()).delete();
             }
+          } catch (IOException ex) {
+            // Ignore all errors trying to update NoteDb at this point. We've
+            // already written the NoteDbChangeState to ReviewDb, which means
+            // if the state is out of date it will be rebuilt the next time it
+            // is needed.
+            log.debug("Ignoring NoteDb update error after ReviewDb write", ex);
+          }
         }
 
         // Reindex changes.

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

@@ -87,6 +87,8 @@ import java.util.concurrent.Callable;
 
 /** View of a single {@link Change} based on the log of its notes branch. */
 public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
+  private static final Logger log = LoggerFactory.getLogger(ChangeNotes.class);
+
   static final Ordering<PatchSetApproval> PSA_BY_TIME =
       Ordering.natural().onResultOf(
         new Function<PatchSetApproval, Timestamp>() {
@@ -113,8 +115,6 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
 
   @Singleton
   public static class Factory {
-    private static final Logger log = LoggerFactory.getLogger(Factory.class);
-
     private final Args args;
     private final Provider<InternalChangeQuery> queryProvider;
     private final ProjectCache projectCache;
@@ -367,6 +367,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
       }
       return ids;
     }
+  }
 
   private static ReviewDb unwrap(ReviewDb db) {
     if (db instanceof DisabledChangesReviewDbWrapper) {
@@ -374,12 +375,11 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     }
     return db;
   }
-  }
 
   private final Project.NameKey project;
-  private final Change change;
   private final RefCache refs;
 
+  private Change change;
   private ImmutableSortedMap<PatchSet.Id, PatchSet> patchSets;
   private ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals;
   private ReviewerSet reviewers;
@@ -634,12 +634,16 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
   private LoadHandle rebuildAndOpen(Repository repo, ObjectId oldId)
       throws IOException {
     try {
-      NoteDbChangeState newState =
-          args.rebuilder.get().rebuild(args.db.get(), getChangeId());
+      NoteDbChangeState newState;
+      try {
+        newState = args.rebuilder.get().rebuild(args.db.get(), getChangeId());
+        repo.scanForRepoChanges();
+      } catch (IOException e) {
+        newState = recheckUpToDate(repo, e);
+      }
       if (newState == null) {
         return super.openHandle(repo, oldId); // May be null in tests.
       }
-      repo.scanForRepoChanges();
       return LoadHandle.create(
           ChangeNotesCommit.newRevWalk(repo), newState.getChangeMetaId());
     } catch (NoSuchChangeException e) {
@@ -648,4 +652,40 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
       throw new IOException(e);
     }
   }
+
+  private NoteDbChangeState recheckUpToDate(Repository repo, IOException e)
+      throws IOException {
+    // Should only be non-null if auto-rebuilding disabled.
+    checkState(refs == null);
+    // An error during auto-rebuilding might be caused by LOCK_FAILURE or a
+    // similar contention issue, where another thread successfully rebuilt the
+    // change. Reread the change from ReviewDb and NoteDb and recheck the state.
+    Change newChange;
+    try {
+      newChange = unwrap(args.db.get()).changes().get(getChangeId());
+    } catch (OrmException e2) {
+      logRecheckError(e2);
+      throw e;
+    }
+    NoteDbChangeState newState = NoteDbChangeState.parse(newChange);
+    boolean upToDate;
+    try {
+      repo.scanForRepoChanges();
+      upToDate = NoteDbChangeState.isChangeUpToDate(
+          newState, new RepoRefCache(repo), getChangeId());
+    } catch (IOException e2) {
+      logRecheckError(e2);
+      throw e;
+    }
+    if (!upToDate) {
+      throw e;
+    }
+    change = new Change(newChange);
+    return newState;
+  }
+
+  private void logRecheckError(Throwable t) {
+    log.error("Error rechecking if change " + getChangeId()
+        + " is up to date; logging this exception but rethrowing original", t);
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java

@@ -24,22 +24,25 @@ import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.server.OrmException;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.Repository;
 
 import java.io.IOException;
 
 public class NoteDbModule extends FactoryModule {
+  private final Config cfg;
   private final boolean useTestBindings;
 
-  static NoteDbModule forTest() {
-    return new NoteDbModule(true);
+  static NoteDbModule forTest(Config cfg) {
+    return new NoteDbModule(cfg, true);
   }
 
-  public NoteDbModule() {
-    this(false);
+  public NoteDbModule(Config cfg) {
+    this(cfg, false);
   }
 
-  private NoteDbModule(boolean useTestBindings) {
+  private NoteDbModule(Config cfg, boolean useTestBindings) {
+    this.cfg = cfg;
     this.useTestBindings = useTestBindings;
   }
 
@@ -50,7 +53,13 @@ public class NoteDbModule extends FactoryModule {
     factory(DraftCommentNotes.Factory.class);
     factory(NoteDbUpdateManager.Factory.class);
     if (!useTestBindings) {
+      if (cfg.getBoolean("noteDb", null, "testRebuilderWrapper", false)) {
+        // Yes, another variety of test bindings with a different way of
+        // configuring it.
+        bind(ChangeRebuilder.class).to(TestChangeRebuilderWrapper.class);
+      } else {
         bind(ChangeRebuilder.class).to(ChangeRebuilderImpl.class);
+      }
     } else {
       bind(ChangeRebuilder.class).toInstance(new ChangeRebuilder(null) {
         @Override

gerrit-server/src/main/java/com/google/gerrit/server/notedb/TestChangeRebuilderWrapper.java

@@ -0,0 +1,86 @@
+// Copyright (C) 2016 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.notedb;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableMultimap;
+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.server.project.NoSuchChangeException;
+import com.google.gwtorm.server.OrmException;
+import com.google.gwtorm.server.SchemaFactory;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Repository;
+
+import java.io.IOException;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+@VisibleForTesting
+@Singleton
+public class TestChangeRebuilderWrapper extends ChangeRebuilder {
+  private final ChangeRebuilderImpl delegate;
+  private final AtomicBoolean stealNextUpdate;
+
+  @Inject
+  TestChangeRebuilderWrapper(SchemaFactory<ReviewDb> schemaFactory,
+      ChangeRebuilderImpl rebuilder) {
+    super(schemaFactory);
+    this.delegate = rebuilder;
+    this.stealNextUpdate = new AtomicBoolean();
+  }
+
+  public void stealNextUpdate() {
+    stealNextUpdate.set(true);
+  }
+
+  @Override
+  public NoteDbChangeState rebuild(ReviewDb db, Change.Id changeId)
+      throws NoSuchChangeException, IOException, OrmException,
+      ConfigInvalidException {
+    NoteDbChangeState result = delegate.rebuild(db, changeId);
+    if (stealNextUpdate.getAndSet(false)) {
+      throw new IOException("Update stolen");
+    }
+    return result;
+  }
+
+  @Override
+  public NoteDbChangeState rebuild(NoteDbUpdateManager manager,
+      ChangeBundle bundle) throws NoSuchChangeException, IOException,
+      OrmException, ConfigInvalidException {
+    // stealNextUpdate doesn't really apply in this case because the IOException
+    // would normally come from the manager.execute() method, which isn't called
+    // here.
+    return delegate.rebuild(manager, bundle);
+  }
+
+  @Override
+  public boolean rebuildProject(ReviewDb db,
+      ImmutableMultimap<Project.NameKey, Change.Id> allChanges,
+      Project.NameKey project, Repository allUsersRepo)
+      throws NoSuchChangeException, IOException, OrmException,
+      ConfigInvalidException {
+    boolean result =
+        delegate.rebuildProject(db, allChanges, project, allUsersRepo);
+    if (stealNextUpdate.getAndSet(false)) {
+      throw new IOException("Update stolen");
+    }
+    return result;
+  }
+}

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

@@ -143,8 +143,9 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests {
     injector = Guice.createInjector(new FactoryModule() {
       @Override
       public void configure() {
+        Config cfg = new Config();
         install(new GitModule());
-        install(NoteDbModule.forTest());
+        install(NoteDbModule.forTest(cfg));
         bind(AllUsersName.class).toProvider(AllUsersNameProvider.class);
         bind(String.class).annotatedWith(GerritServerId.class)
             .toInstance("gerrit");
@@ -154,7 +155,7 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests {
         bind(CapabilityControl.Factory.class)
             .toProvider(Providers.<CapabilityControl.Factory> of(null));
         bind(Config.class).annotatedWith(GerritServerConfig.class)
-            .toInstance(new Config());
+            .toInstance(cfg);
         bind(String.class).annotatedWith(AnonymousCowardName.class)
             .toProvider(AnonymousCowardNameProvider.class);
         bind(String.class).annotatedWith(CanonicalWebUrl.class)