Manage change deletion performed by pull-replication When a delete change is replicated through the pull-replication plugin, the fetch-ref-replicated event contains the ref-name of the change /meta with a FORCED|NO_CHANGE result and an inexistent ref on the repository. Leverage the event in the FetchRefReplicatedEventHandler and delete also the change from the index. Depends-On: https://gerrit-review.googlesource.com/c/plugins/replication/+/480184? Change-Id: I8567a583fc149eb630c72eb45084a5e7b423decb
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..83076bf 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
@@ -21,21 +21,30 @@ import com.google.gerrit.server.config.GerritInstanceId; import com.google.gerrit.server.events.Event; import com.google.gerrit.server.events.EventListener; +import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.index.change.ChangeIndexer; import com.google.inject.Inject; 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 java.io.IOException; +import org.eclipse.jgit.lib.RefUpdate; +import org.eclipse.jgit.lib.Repository; public class FetchRefReplicatedEventHandler implements EventListener { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + private final GitRepositoryManager gitRepositoryManager; private ChangeIndexer changeIndexer; private final String instanceId; @Inject - FetchRefReplicatedEventHandler(ChangeIndexer changeIndexer, @GerritInstanceId String instanceId) { + FetchRefReplicatedEventHandler( + ChangeIndexer changeIndexer, + @GerritInstanceId String instanceId, + GitRepositoryManager gitRepositoryManager) { this.changeIndexer = changeIndexer; this.instanceId = instanceId; + this.gitRepositoryManager = gitRepositoryManager; } @Override @@ -46,7 +55,15 @@ if (event instanceof FetchRefReplicatedEvent && isLocalEvent(event)) { FetchRefReplicatedEvent fetchRefReplicatedEvent = (FetchRefReplicatedEvent) event; - if (!RefNames.isNoteDbMetaRef(fetchRefReplicatedEvent.getRefName()) + // This removal of the ':' prefix is needed because of Issue 426470297 in the + // pull-replication plugin + // that may leave the deleted refs notification as a ref-spec instead of the deleted + // ref-name. + String refName = + fetchRefReplicatedEvent.ref.startsWith(":") + ? fetchRefReplicatedEvent.ref.substring(1) + : fetchRefReplicatedEvent.ref; + if (!RefNames.isNoteDbMetaRef(refName) || !fetchRefReplicatedEvent .getStatus() .equals(ReplicationState.RefFetchResult.SUCCEEDED.toString())) { @@ -54,18 +71,44 @@ } Project.NameKey projectNameKey = fetchRefReplicatedEvent.getProjectNameKey(); - logger.atFine().log( - "Indexing ref '%s' for project %s", - fetchRefReplicatedEvent.getRefName(), projectNameKey.get()); - Change.Id changeId = Change.Id.fromRef(fetchRefReplicatedEvent.getRefName()); - if (changeId != null) { - changeIndexer.index(projectNameKey, changeId); - } else { - logger.atWarning().log( - "Couldn't get changeId from refName. Skipping indexing of change %s for project %s", - fetchRefReplicatedEvent.getRefName(), projectNameKey.get()); + Change.Id changeId = Change.Id.fromRef(refName); + + try (Repository repo = gitRepositoryManager.openRepository(projectNameKey)) { + if (changeId != null) { + RefUpdate.Result fetchRefReplicatedEventResult = + fetchRefReplicatedEvent.getRefUpdateResult(); + if ((RefUpdate.Result.FORCED.equals(fetchRefReplicatedEventResult) + || RefUpdate.Result.NO_CHANGE.equals(fetchRefReplicatedEventResult)) + && repo.exactRef(refName) == null) { + logger.atInfo().log( + "Deleting change '%s' from index following the forced delete of '%s' on project" + + " %s", + changeId, refName, projectNameKey.get()); + // NOTE: The change has been already deleted and we just don't know if it was a + // locally + // created change or imported, because the meta-data is not there anymore. + // In the future, it would be best to have that information passed to the event by the + // pull-replication plugin *before* the delete, and added to the + // FetchRefReplicatedEvent, + // so that multi-site can use it for computing the virtual change-id and assuring the + // delete + // of the imported changes as well. + changeIndexer.delete(changeId); + } else { + logger.atInfo().log( + "Indexing change '%s' following update of '%s' on project %s", + changeId, refName, projectNameKey.get()); + changeIndexer.index(projectNameKey, changeId); + } + } else { + logger.atWarning().log( + "Couldn't get changeId from refName. Skipping indexing of change %s for project %s", + refName, projectNameKey.get()); + } } } + } catch (IOException e) { + logger.atSevere().withCause(e).log("Unable to process event %s", event); } finally { Context.setForwardedEvent(isForwarded); }
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..ed2f958 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
@@ -16,6 +16,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -23,12 +24,14 @@ import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Project; +import com.google.gerrit.server.git.GitRepositoryManager; 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.lib.RefUpdate; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.transport.URIish; import org.junit.Before; import org.junit.Test; @@ -36,20 +39,46 @@ public class FetchRefReplicatedEventHandlerTest { private static final String LOCAL_INSTANCE_ID = "local-instance-id"; private static final String REMOTE_INSTANCE_ID = "remote-instance-id"; + private static final Project.NameKey PROJECT_NAME_KEY = Project.nameKey("testProject"); private ChangeIndexer changeIndexerMock; + private GitRepositoryManager gitRepositoryManager; + private Repository repository; private FetchRefReplicatedEventHandler fetchRefReplicatedEventHandler; private static URIish sourceUri; @Before public void setUp() throws Exception { changeIndexerMock = mock(ChangeIndexer.class); + gitRepositoryManager = mock(GitRepositoryManager.class); + repository = mock(Repository.class); + doReturn(repository).when(gitRepositoryManager).openRepository(any()); fetchRefReplicatedEventHandler = - new FetchRefReplicatedEventHandler(changeIndexerMock, LOCAL_INSTANCE_ID); + new FetchRefReplicatedEventHandler( + changeIndexerMock, LOCAL_INSTANCE_ID, gitRepositoryManager); sourceUri = new URIish("git://aSourceNode/testProject.git"); } @Test public void onEventShouldIndexExistingChange() { + String ref = "refs/changes/41/41/meta"; + Change.Id changeId = Change.Id.fromRef(ref); + try { + Context.setLocalEvent(true); + fetchRefReplicatedEventHandler.onEvent( + newFetchRefReplicatedEvent( + PROJECT_NAME_KEY, + ref, + ReplicationState.RefFetchResult.SUCCEEDED, + LOCAL_INSTANCE_ID, + RefUpdate.Result.FAST_FORWARD)); + verify(changeIndexerMock, times(1)).index(eq(PROJECT_NAME_KEY), eq(changeId)); + } finally { + Context.unsetLocalEvent(); + } + } + + @Test + public void onEventShouldDeleteChangeFromIndex() { Project.NameKey projectNameKey = Project.nameKey("testProject"); String ref = "refs/changes/41/41/meta"; Change.Id changeId = Change.Id.fromRef(ref); @@ -57,8 +86,12 @@ Context.setLocalEvent(true); fetchRefReplicatedEventHandler.onEvent( newFetchRefReplicatedEvent( - projectNameKey, ref, ReplicationState.RefFetchResult.SUCCEEDED, LOCAL_INSTANCE_ID)); - verify(changeIndexerMock, times(1)).index(eq(projectNameKey), eq(changeId)); + projectNameKey, + ref, + ReplicationState.RefFetchResult.SUCCEEDED, + LOCAL_INSTANCE_ID, + RefUpdate.Result.FORCED)); + verify(changeIndexerMock, times(1)).delete(eq(changeId)); } finally { Context.unsetLocalEvent(); } @@ -71,7 +104,11 @@ Change.Id changeId = Change.Id.fromRef(ref); fetchRefReplicatedEventHandler.onEvent( newFetchRefReplicatedEvent( - projectNameKey, ref, ReplicationState.RefFetchResult.SUCCEEDED, REMOTE_INSTANCE_ID)); + projectNameKey, + ref, + ReplicationState.RefFetchResult.SUCCEEDED, + REMOTE_INSTANCE_ID, + RefUpdate.Result.FAST_FORWARD)); verify(changeIndexerMock, never()).index(eq(projectNameKey), eq(changeId)); } @@ -82,7 +119,11 @@ Change.Id changeId = Change.Id.fromRef(ref); fetchRefReplicatedEventHandler.onEvent( newFetchRefReplicatedEvent( - projectNameKey, ref, ReplicationState.RefFetchResult.SUCCEEDED, LOCAL_INSTANCE_ID)); + projectNameKey, + ref, + ReplicationState.RefFetchResult.SUCCEEDED, + LOCAL_INSTANCE_ID, + RefUpdate.Result.FAST_FORWARD)); verify(changeIndexerMock, never()).index(eq(projectNameKey), eq(changeId)); } @@ -93,7 +134,8 @@ Project.nameKey("testProject"), "invalidRef", ReplicationState.RefFetchResult.SUCCEEDED, - LOCAL_INSTANCE_ID)); + LOCAL_INSTANCE_ID, + RefUpdate.Result.FAST_FORWARD)); verify(changeIndexerMock, never()).index(any(), any()); } @@ -103,7 +145,11 @@ String ref = "refs/changes/41/41/meta"; fetchRefReplicatedEventHandler.onEvent( newFetchRefReplicatedEvent( - projectNameKey, ref, ReplicationState.RefFetchResult.FAILED, LOCAL_INSTANCE_ID)); + projectNameKey, + ref, + ReplicationState.RefFetchResult.FAILED, + LOCAL_INSTANCE_ID, + RefUpdate.Result.FAST_FORWARD)); verify(changeIndexerMock, never()).index(any(), any()); } @@ -113,15 +159,23 @@ String ref = "refs/changes/41/41/meta"; fetchRefReplicatedEventHandler.onEvent( newFetchRefReplicatedEvent( - projectNameKey, ref, ReplicationState.RefFetchResult.NOT_ATTEMPTED, LOCAL_INSTANCE_ID)); + projectNameKey, + ref, + ReplicationState.RefFetchResult.NOT_ATTEMPTED, + LOCAL_INSTANCE_ID, + RefUpdate.Result.FAST_FORWARD)); verify(changeIndexerMock, never()).index(any(), any()); } private FetchRefReplicatedEvent newFetchRefReplicatedEvent( - Project.NameKey projectNameKey, String ref, RefFetchResult fetchResult, String instanceId) { + Project.NameKey projectNameKey, + String ref, + RefFetchResult fetchResult, + String instanceId, + RefUpdate.Result refUpdateResult) { FetchRefReplicatedEvent event = new FetchRefReplicatedEvent( - projectNameKey.get(), ref, sourceUri, fetchResult, RefUpdate.Result.FAST_FORWARD); + projectNameKey.get(), ref, sourceUri, fetchResult, refUpdateResult); event.instanceId = instanceId; return event; }