Do not fail reindexing changes when the patch-sets aren't replicated yet Depending on how replication is configured, the changes' /meta refs could be replicated before their associated patch sets. Every time a change is replicated, the pull-replication plugin emits a fetch-ref-replicated event related to the change's refs. When creating a new change, two refs are replicated: 1. refs/changes/<NN>/<NNNN>/1 (first patch-set of the change) 2. refs/changes/<NN>/<NNNN>/meta (change meta-ref) When the above refs are replicated out of order, with 2. replicated before 1., then the multi-site plugin tried to reindex the change in an inconsistent state, throwing a "missing unknown" error and resulting with a stale index. Instead of reacting only to 'meta' refs replication, consider all the refs' replications events associated with a change and ignore the Indexing errors associated with a missing or unknown SHA1. Bug: Issue 425059627 Change-Id: I9a6337c1cecb52df832d64b2c7bb30e97ba7916a
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/index/FetchRefReplicatedEventHandler.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/index/FetchRefReplicatedEventHandler.java index d45f526..f47cea5 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/multisite/index/FetchRefReplicatedEventHandler.java +++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/index/FetchRefReplicatedEventHandler.java
@@ -18,6 +18,7 @@ import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Project; import com.google.gerrit.entities.RefNames; +import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.server.config.GerritInstanceId; import com.google.gerrit.server.events.Event; import com.google.gerrit.server.events.EventListener; @@ -26,6 +27,7 @@ import com.googlesource.gerrit.plugins.multisite.forwarder.Context; import com.googlesource.gerrit.plugins.replication.pull.FetchRefReplicatedEvent; import com.googlesource.gerrit.plugins.replication.pull.ReplicationState; +import org.eclipse.jgit.errors.MissingObjectException; public class FetchRefReplicatedEventHandler implements EventListener { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); @@ -46,7 +48,8 @@ if (event instanceof FetchRefReplicatedEvent && isLocalEvent(event)) { FetchRefReplicatedEvent fetchRefReplicatedEvent = (FetchRefReplicatedEvent) event; - if (!RefNames.isNoteDbMetaRef(fetchRefReplicatedEvent.getRefName()) + boolean isMetaRef = RefNames.isNoteDbMetaRef(fetchRefReplicatedEvent.getRefName()); + if (!RefNames.isRefsChanges(fetchRefReplicatedEvent.getRefName()) || !fetchRefReplicatedEvent .getStatus() .equals(ReplicationState.RefFetchResult.SUCCEEDED.toString())) { @@ -58,7 +61,15 @@ "Indexing ref '%s' for project %s", fetchRefReplicatedEvent.getRefName(), projectNameKey.get()); Change.Id changeId = Change.Id.fromRef(fetchRefReplicatedEvent.getRefName()); - if (changeId != null) { + if (changeId != null && isMetaRef) { + catchStorageExceptionForMissingUnknown( + () -> changeIndexer.index(projectNameKey, changeId), + "Skipping indexing of " + + projectNameKey + + "~" + + changeId.get() + + " because its patch-sets aren't replicated yet"); + } else if (changeId != null) { changeIndexer.index(projectNameKey, changeId); } else { logger.atWarning().log( @@ -71,6 +82,23 @@ } } + private void catchStorageExceptionForMissingUnknown(Runnable lambda, String infoMessage) + throws RuntimeException { + try { + lambda.run(); + } catch (StorageException se) { + Throwable cause = se.getCause(); + while (cause != null) { + if (cause instanceof MissingObjectException) { + logger.atInfo().log("%s", infoMessage); + return; + } + cause = cause.getCause(); + } + throw se; + } + } + private boolean isLocalEvent(Event event) { return instanceId.equals(event.instanceId); }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/index/FetchRefReplicatedEventHandlerTest.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/index/FetchRefReplicatedEventHandlerTest.java index 47b6072..e7e0d8a 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/multisite/index/FetchRefReplicatedEventHandlerTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/index/FetchRefReplicatedEventHandlerTest.java
@@ -14,8 +14,11 @@ package com.googlesource.gerrit.plugins.multisite.index; +import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.testing.GerritJUnit.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -23,11 +26,14 @@ import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Project; +import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.server.index.change.ChangeIndexer; import com.googlesource.gerrit.plugins.replication.pull.Context; import com.googlesource.gerrit.plugins.replication.pull.FetchRefReplicatedEvent; import com.googlesource.gerrit.plugins.replication.pull.ReplicationState; import com.googlesource.gerrit.plugins.replication.pull.ReplicationState.RefFetchResult; +import org.eclipse.jgit.errors.MissingObjectException; +import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.transport.URIish; import org.junit.Before; @@ -65,6 +71,57 @@ } @Test + public void onEventShouldIgnoreMissingObjectWhenIndexExistingChangeMeta() { + Project.NameKey projectNameKey = Project.nameKey("testProject"); + String ref = "refs/changes/41/41/meta"; + Change.Id changeId = Change.Id.fromRef(ref); + try { + Context.setLocalEvent(true); + StorageException storageExceptionForMissingObject = + new StorageException( + "Test storage exception", + new MissingObjectException(ObjectId.zeroId(), "Missing unknown")); + doThrow(storageExceptionForMissingObject) + .when(changeIndexerMock) + .index(eq(projectNameKey), eq(changeId)); + fetchRefReplicatedEventHandler.onEvent( + newFetchRefReplicatedEvent( + projectNameKey, ref, ReplicationState.RefFetchResult.SUCCEEDED, LOCAL_INSTANCE_ID)); + verify(changeIndexerMock, times(1)).index(eq(projectNameKey), eq(changeId)); + } finally { + Context.unsetLocalEvent(); + } + } + + @Test + public void onEventShouldThrowStorageExceptionWhenIndexPatchSetsWithMissingRefs() { + Project.NameKey projectNameKey = Project.nameKey("testProject"); + String ref = "refs/changes/41/41/1"; + Change.Id changeId = Change.Id.fromRef(ref); + try { + Context.setLocalEvent(true); + StorageException storageExceptionForMissingObject = + new StorageException( + "Test storage exception", + new MissingObjectException(ObjectId.zeroId(), "Missing unknown")); + doThrow(storageExceptionForMissingObject) + .when(changeIndexerMock) + .index(eq(projectNameKey), eq(changeId)); + StorageException indexException = + assertThrows( + StorageException.class, + () -> + fetchRefReplicatedEventHandler.onEvent( + newFetchRefReplicatedEvent( + projectNameKey, ref, RefFetchResult.SUCCEEDED, LOCAL_INSTANCE_ID))); + assertThat(indexException).isEqualTo(storageExceptionForMissingObject); + verify(changeIndexerMock, times(1)).index(eq(projectNameKey), eq(changeId)); + } finally { + Context.unsetLocalEvent(); + } + } + + @Test public void onEventShouldNotIndexIfNotLocalEvent() { Project.NameKey projectNameKey = Project.nameKey("testProject"); String ref = "refs/changes/41/41/meta"; @@ -76,14 +133,14 @@ } @Test - public void onEventShouldIndexOnlyMetaRef() { + public void onEventShouldIndexOnPatchSetsRef() { Project.NameKey projectNameKey = Project.nameKey("testProject"); String ref = "refs/changes/41/41/1"; Change.Id changeId = Change.Id.fromRef(ref); fetchRefReplicatedEventHandler.onEvent( newFetchRefReplicatedEvent( projectNameKey, ref, ReplicationState.RefFetchResult.SUCCEEDED, LOCAL_INSTANCE_ID)); - verify(changeIndexerMock, never()).index(eq(projectNameKey), eq(changeId)); + verify(changeIndexerMock).index(eq(projectNameKey), eq(changeId)); } @Test