NoteDbMigrator: Totally skip migration for orphan changes
This case was explicitly handled by ChangeRebuilderImpl, and
NoteDbMigrator in fact caught RepositoryNotFoundException and let the
migration proceed. Unfortunately, this would then cause the primary
storage migration step to fail with:
com.google.gwtorm.server.OrmRuntimeException: change X has no note_db_state; rebuild it first
Work around this by catching this exception and checking after the fact
whether the change is known to be unrecoverably corrupt. There is
already another check in place: no patch set, in which case the
migration process would also proceed, that was added in I0439c7fc8e5.
So that we are just adding yet another "supported" kind of change
corruption recovery mechanism to the migration proccess.
Also add a test that is trying to migrate two different changes from two
different projects, where one project was deleted on the file system.
That test demonstrates, that one change is completely migrated while
for another change the migration is skipped, as it is detected as
corrupted change.
The usual use case for orphan changes, also, the changes with not
existing git repsitories on the file system, is often seen for gerrit
test instances: Database replicated from the production system but only
a couple of git repositories are replicated, leading to orphan changes.
Reported-by: Alan Tokaev <tokaev@gmail.com>
Bug: Issue 12097
Change-Id: I516f40f03feaafe3014fabb0c2f5c40d6753b8bc
diff --git a/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java b/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
index ebec9c5..d6daae1 100644
--- a/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
+++ b/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
@@ -72,6 +72,7 @@
import com.google.gerrit.server.notedb.RepoSequence;
import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder.NoPatchSetsException;
import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.update.ChainedReceiveCommands;
import com.google.gerrit.server.update.RefUpdateUtil;
import com.google.gerrit.server.util.ManualRequestContext;
@@ -164,6 +165,7 @@
private final MutableNotesMigration globalNotesMigration;
private final PrimaryStorageMigrator primaryStorageMigrator;
private final DynamicSet<NotesMigrationStateListener> listeners;
+ private final ProjectCache projectCache;
private int threads;
private ImmutableList<Project.NameKey> projects = ImmutableList.of();
@@ -193,7 +195,8 @@
WorkQueue workQueue,
MutableNotesMigration globalNotesMigration,
PrimaryStorageMigrator primaryStorageMigrator,
- DynamicSet<NotesMigrationStateListener> listeners) {
+ DynamicSet<NotesMigrationStateListener> listeners,
+ ProjectCache projectCache) {
// Reload gerrit.config/notedb.config on each migrator invocation, in case a previous
// migration in the same process modified the on-disk contents. This ensures the defaults for
// trial/autoMigrate get set correctly below.
@@ -213,6 +216,7 @@
this.globalNotesMigration = globalNotesMigration;
this.primaryStorageMigrator = primaryStorageMigrator;
this.listeners = listeners;
+ this.projectCache = projectCache;
this.trial = getTrialMode(cfg);
this.autoMigrate = getAutoMigrate(cfg);
}
@@ -400,6 +404,7 @@
changes,
progressOut,
stopAtState,
+ projectCache,
trial,
forceRebuild,
sequenceGap >= 0 ? sequenceGap : Sequences.getChangeSequenceGap(cfg),
@@ -429,6 +434,7 @@
private final ImmutableList<Change.Id> changes;
private final OutputStream progressOut;
private final NotesMigrationState stopAtState;
+ private final ProjectCache projectCache;
private final boolean trial;
private final boolean forceRebuild;
private final int sequenceGap;
@@ -455,6 +461,7 @@
ImmutableList<Change.Id> changes,
OutputStream progressOut,
NotesMigrationState stopAtState,
+ ProjectCache projectCache,
boolean trial,
boolean forceRebuild,
int sequenceGap,
@@ -489,6 +496,7 @@
this.changes = changes;
this.progressOut = progressOut;
this.stopAtState = stopAtState;
+ this.projectCache = projectCache;
this.trial = trial;
this.forceRebuild = forceRebuild;
this.sequenceGap = sequenceGap;
@@ -702,11 +710,13 @@
* of the NoteDb migration code, which is too risky to attempt in the stable branch where this bug
* had to be fixed.
*
- * <p>As of this writing, the only case where this happens is when a change has no patch sets.
+ * <p>As of this writing, there are only two cases where this happens: when a change has no patch
+ * sets, or the project doesn't exist.
*/
- private static boolean canSkipPrimaryStorageMigration(ReviewDb db, Change.Id id) {
+ private boolean canSkipPrimaryStorageMigration(ReviewDb db, Change.Id id) {
try {
- return Iterables.isEmpty(unwrapDb(db).patchSets().byChange(id));
+ return Iterables.isEmpty(unwrapDb(db).patchSets().byChange(id))
+ || projectCache.get(unwrapDb(db).changes().get(id).getProject()) == null;
} catch (Exception e) {
logger.atSevere().withCause(e).log(
"Error checking if change %s can be skipped, assuming no", id);
diff --git a/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java b/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
index c249973..b9ed0f3 100644
--- a/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
@@ -35,6 +35,8 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSortedSet;
+import com.google.common.io.MoreFiles;
+import com.google.common.io.RecursiveDeleteOption;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GerritConfig;
import com.google.gerrit.acceptance.NoHttpd;
@@ -84,6 +86,7 @@
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.lib.RepositoryCache.FileKey;
import org.eclipse.jgit.storage.file.FileBasedConfig;
import org.eclipse.jgit.util.FS;
import org.junit.After;
@@ -511,6 +514,42 @@
}
@Test
+ public void fullMigrationOneChangeWithNoProject() throws Exception {
+ PushOneCommit.Result r1 = createChange();
+ Change.Id id1 = r1.getChange().getId();
+
+ Project.NameKey p2 = createProject("project2");
+ TestRepository<?> tr2 = cloneProject(p2, admin);
+ PushOneCommit.Result r2 = pushFactory.create(db, admin.getIdent(), tr2).to("refs/for/master");
+ Change.Id id2 = r2.getChange().getId();
+
+ // TODO(davido): Find an easier way to wipe out a repository from the file system.
+ MoreFiles.deleteRecursively(
+ FileKey.lenient(
+ sitePaths
+ .resolve(cfg.getString("gerrit", null, "basePath"))
+ .resolve(p2.get())
+ .toFile(),
+ FS.DETECTED)
+ .getFile()
+ .toPath(),
+ RecursiveDeleteOption.ALLOW_INSECURE);
+
+ migrate(b -> b);
+ assertNotesMigrationState(NOTE_DB, false, false);
+
+ try (ReviewDb db = schemaFactory.open();
+ Repository repo = repoManager.openRepository(project)) {
+ assertThat(repo.exactRef(RefNames.changeMetaRef(id1))).isNotNull();
+ assertThat(db.changes().get(id1).getNoteDbState()).isEqualTo(NOTE_DB_PRIMARY_STATE);
+ }
+
+ // A change without project is so corrupt that it is completely skipped by the migration
+ // process.
+ assertThat(db.changes().get(id2).getNoteDbState()).isNull();
+ }
+
+ @Test
public void fullMigrationMissingPatchSetRefs() throws Exception {
PushOneCommit.Result r = createChange();
Change.Id id = r.getChange().getId();
