Filter excluded refs before scheduling tasks excludedRefsPattern was only applied when building the push, so matching refs were still queued and written to task storage as no-op work. Check the pattern in Destination.wouldPushRef() so those refs are skipped before a task is scheduled or persisted. Change-Id: Iebfca8a5d025bef57894d67578668422f05a56ad
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java b/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java index b35a5d1..08a6c62 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java
@@ -85,7 +85,6 @@ import java.util.concurrent.locks.ReadWriteLock; import java.util.function.Function; import java.util.function.Supplier; -import java.util.regex.Pattern; import java.util.stream.Collectors; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.Ref; @@ -821,6 +820,10 @@ if (PushOne.ALL_REFS.equals(ref)) { return true; } + if (isRefExcluded(ref)) { + repLog.atFine().log("Skipping push of ref %s; it matches excludedRefsPattern", ref); + return false; + } for (RefSpec s : config.getRemoteConfig().getPushRefSpecs()) { if (s.matchSource(ref)) { return true; @@ -975,8 +978,8 @@ return config.replicateNoteDbMetaRefs(); } - ImmutableList<Pattern> excludedRefsPattern() { - return config.excludedRefsPattern(); + boolean isRefExcluded(String ref) { + return config.isRefExcluded(ref); } boolean storeRefLog() {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java b/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java index 24f12e3..07cb830 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java
@@ -795,7 +795,7 @@ return !(noPerms && RefNames.REFS_CONFIG.equals(ref)) && !ref.startsWith(RefNames.REFS_CACHE_AUTOMERGE) && !(!pool.replicateNoteDbMetaRefs() && RefNames.isNoteDbMetaRef(ref)) - && pool.excludedRefsPattern().stream().noneMatch(p -> p.matcher(ref).matches()); + && !pool.isRefExcluded(ref); } private Map<String, Ref> listRemote(Transport tn)
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/RemoteConfiguration.java b/src/main/java/com/googlesource/gerrit/plugins/replication/RemoteConfiguration.java index 79bdbf3..108250b 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/RemoteConfiguration.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/RemoteConfiguration.java
@@ -161,6 +161,16 @@ } /** + * Whether a ref is excluded from replication by any of the {@link #excludedRefsPattern()} + * + * @param ref name of the ref to check + * @return true if the ref should not be replicated, false otherwise + */ + default boolean isRefExcluded(String ref) { + return excludedRefsPattern().stream().anyMatch(p -> p.matcher(ref).matches()); + } + + /** * reflog storage flag for newly created repositories * * @return true if new repositories should store ref-updates in their reflog
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md index 211bbcb..76a1858 100644 --- a/src/main/resources/Documentation/config.md +++ b/src/main/resources/Documentation/config.md
@@ -738,6 +738,9 @@ `excludedRefsPattern` keys can be supplied, to specify multiple regular expressions to match against. + Excluded refs are filtered out before a replication task is scheduled, so they + do not appear in the replication queue or in the persisted task storage. + Do not exclude any refs by default. remote.NAME.urlDistributionStrategy
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/PushOneTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/PushOneTest.java index 6def78f..076a903 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/PushOneTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/PushOneTest.java
@@ -53,7 +53,6 @@ import java.util.concurrent.Callable; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; -import java.util.regex.Pattern; import org.eclipse.jgit.errors.NotSupportedException; import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.eclipse.jgit.errors.TransportException; @@ -297,9 +296,8 @@ @Test public void skipPushingExcludedRefs() throws InterruptedException, IOException { - when(destinationMock.excludedRefsPattern()) - .thenReturn( - ImmutableList.of(Pattern.compile("refs/foo/.*"), Pattern.compile("refs/bar/.*"))); + when(destinationMock.isRefExcluded("refs/foo/test")).thenReturn(true); + when(destinationMock.isRefExcluded("refs/bar/test")).thenReturn(true); PushOne pushOne = Mockito.spy(createPushOne(null)); Ref ref1 = @@ -496,7 +494,6 @@ private void setupDestinationMock() { destinationMock = mock(Destination.class); when(destinationMock.requestRunway(any())).thenReturn(RunwayStatus.allowed()); - when(destinationMock.excludedRefsPattern()).thenReturn(ImmutableList.of()); } private void setupPermissionBackedMock() {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationStorageIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationStorageIT.java index bfcf5be..08a7ecb 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationStorageIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationStorageIT.java
@@ -131,6 +131,21 @@ } @Test + public void shouldNotCreateReplicationTaskForRefMatchingExcludedRefsPattern() throws Exception { + createTestProject(project + "replica"); + setExcludedRefsPattern("foo", "refs/heads/excluded.*"); + + scheduleFullSync("refs/heads/excluded-branch"); + + assertThat(listWaiting()).isEmpty(); + + String replicatedRef = "refs/heads/replicated-branch"; + scheduleFullSync(replicatedRef); + + assertThat(listWaitingReplicationTasks(Pattern.quote(replicatedRef))).hasSize(1); + } + + @Test public void shouldFirePendingOnlyToIncompleteUri() throws Exception { String suffix1 = "replica1"; String suffix2 = "replica2"; @@ -377,6 +392,20 @@ assertThat(listWaitingReplicationTasks(branchToDelete)).hasSize(1); } + private void setExcludedRefsPattern(String remote, String pattern) throws Exception { + setReplicationDestination(remote, "replica", ALL_PROJECTS, Integer.MAX_VALUE); + config.setString("remote", remote, "excludedRefsPattern", pattern); + config.save(); + reloadConfig(); + } + + private void scheduleFullSync(String ref) { + plugin + .getSysInjector() + .getInstance(ReplicationQueue.class) + .scheduleFullSync(project, null, ref, Set.of(), new ReplicationState(NO_OP), false); + } + private boolean isTaskRescheduled(Queue queue, URIish uri) { PushOne pushOne = queue.pending.get(uri); return pushOne == null ? false : pushOne.isRetrying();