diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java index ea4909f8ab..27f1c51f18 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java +++ b/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; + @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); diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/BatchProgramModule.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/BatchProgramModule.java index be061c725e..52a3b788f5 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/BatchProgramModule.java +++ b/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()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java index 5bbfd3d498..c516f4a450 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/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()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java index 3b3c78e9df..6a2ba0fd19 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java +++ b/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,13 +599,21 @@ public class BatchUpdate implements AutoCloseable { db.rollback(); } - // Execute NoteDb updates after committing ReviewDb updates. if (notesMigration.writeChanges()) { - if (updateManager != null) { - updateManager.execute(); - } - if (ctx.deleted) { - new ChangeDelete(plcUtil, getRepository(), ctx.getNotes()).delete(); + 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); } } 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 926194b9d5..0e1aa45cef 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 @@ -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 { + private static final Logger log = LoggerFactory.getLogger(ChangeNotes.class); + static final Ordering PSA_BY_TIME = Ordering.natural().onResultOf( new Function() { @@ -113,8 +115,6 @@ public class ChangeNotes extends AbstractChangeNotes { @Singleton public static class Factory { - private static final Logger log = LoggerFactory.getLogger(Factory.class); - private final Args args; private final Provider queryProvider; private final ProjectCache projectCache; @@ -367,19 +367,19 @@ public class ChangeNotes extends AbstractChangeNotes { } return ids; } + } - private static ReviewDb unwrap(ReviewDb db) { - if (db instanceof DisabledChangesReviewDbWrapper) { - db = ((DisabledChangesReviewDbWrapper) db).unsafeGetDelegate(); - } - return db; + private static ReviewDb unwrap(ReviewDb db) { + if (db instanceof DisabledChangesReviewDbWrapper) { + db = ((DisabledChangesReviewDbWrapper) db).unsafeGetDelegate(); } + return db; } private final Project.NameKey project; - private final Change change; private final RefCache refs; + private Change change; private ImmutableSortedMap patchSets; private ImmutableListMultimap approvals; private ReviewerSet reviewers; @@ -634,12 +634,16 @@ public class ChangeNotes extends AbstractChangeNotes { 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 { 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); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java index 99c5fa63ab..485c22f9ad 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java +++ b/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) { - bind(ChangeRebuilder.class).to(ChangeRebuilderImpl.class); + 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 diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/TestChangeRebuilderWrapper.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/TestChangeRebuilderWrapper.java new file mode 100644 index 0000000000..f6351be055 --- /dev/null +++ b/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 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 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; + } +} diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java index 68c79a0c17..b361388b8e 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java +++ b/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. 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)