Merge branch 'stable-3.14' * stable-3.14: Retry failovers by applying url distribution strategy Add projectSharded URL selection per remote Change-Id: Id760dc515503ea27f6720ddf9edec00b0e159e19
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 c1a1936..f17b50b 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java
@@ -877,11 +877,11 @@ } List<URIish> getDistributedUris(Project.NameKey project, String urlMatch) { - return getDistributedUris(getURIs(project, urlMatch)); + return getDistributedUris(project, getURIs(project, urlMatch)); } - List<URIish> getDistributedUris(List<URIish> candidates) { - return urlDistributor.select(candidates); + List<URIish> getDistributedUris(Project.NameKey project, List<URIish> candidates) { + return urlDistributor.select(project, candidates); } URIish getURI(URIish template, Project.NameKey project) throws URISyntaxException {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationsCollection.java b/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationsCollection.java index 4896dcb..f752a4a 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationsCollection.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationsCollection.java
@@ -145,7 +145,7 @@ validUris.add(uri); } } - config.getDistributedUris(validUris).forEach(uri -> uris.put(config, uri)); + config.getDistributedUris(projectName, validUris).forEach(uri -> uris.put(config, uri)); } return uris; }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/UrlDistributionStrategy.java b/src/main/java/com/googlesource/gerrit/plugins/replication/UrlDistributionStrategy.java index 1230e36..50d6504 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/UrlDistributionStrategy.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/UrlDistributionStrategy.java
@@ -14,7 +14,11 @@ package com.googlesource.gerrit.plugins.replication; +import com.google.common.collect.ImmutableList; +import com.google.gerrit.entities.Project; +import com.google.gerrit.entities.Project.NameKey; import java.util.Arrays; +import java.util.Comparator; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; import org.eclipse.jgit.transport.URIish; @@ -33,7 +37,7 @@ ALL("all") { @Override public Instance newInstance() { - return candidates -> candidates; + return (project, candidates) -> candidates; } }, @@ -51,7 +55,7 @@ private final AtomicInteger index = new AtomicInteger(); @Override - public List<URIish> select(List<URIish> candidates) { + public List<URIish> select(NameKey project, List<URIish> candidates) { if (candidates.isEmpty()) { return List.of(); } @@ -73,6 +77,53 @@ } }; } + }, + + /** + * Push to exactly one URL, chosen by hashing the project name so that a given project always maps + * to the same URL. Like {@link #ROUND_ROBIN} this writes each push only once, which matters when + * the replica hosts share a single backend, but it additionally keeps consecutive updates for one + * project on the same URL. Because replication tasks are coalesced per (project, URI), rotating + * URLs would let successive updates for one project run as separate tasks racing against the same + * backend; pinning the project collapses them into a single task and keeps the receiving host's + * caches warm. + * + * <p>The candidates are sorted before indexing so that the mapping does not depend on the order + * in which the URLs happen to be configured. Together with {@link Project.NameKey#hashCode()}, + * which is the specified {@link String#hashCode()} of the project name, this makes every host + * reading the same config agree on the mapping. + */ + PROJECT_SHARDED("projectSharded") { + @Override + public Instance newInstance() { + return new Instance() { + @Override + public List<URIish> select(NameKey project, List<URIish> candidates) { + if (candidates.isEmpty()) { + return List.of(); + } + ImmutableList<URIish> sorted = sortedByUrl(candidates); + return List.of(sorted.get(Math.floorMod(project.hashCode(), sorted.size()))); + } + + @Override + public URIish failover(List<URIish> candidates, URIish failed) { + if (candidates.size() < 2) { + return failed; + } + ImmutableList<URIish> sorted = sortedByUrl(candidates); + int failedIndex = sorted.indexOf(failed); + if (failedIndex < 0) { + return failed; + } + return sorted.get((failedIndex + 1) % sorted.size()); + } + + private ImmutableList<URIish> sortedByUrl(List<URIish> candidates) { + return ImmutableList.sortedCopyOf(Comparator.comparing(URIish::toString), candidates); + } + }; + } }; public final String configKey; @@ -98,12 +149,18 @@ /** A stateful executor for a {@link UrlDistributionStrategy} strategy. */ @FunctionalInterface public interface Instance { - /** Select the URLs to push to for this scheduling event. */ - List<URIish> select(List<URIish> candidates); + /** + * Selects the URLs to push to out of the candidates for the given project. + * + * @param project project being replicated, used by project-affine strategies. + * @param candidates URLs the project could be pushed to. + * @return the subset of candidates to push to. + */ + List<URIish> select(Project.NameKey project, List<URIish> candidates); /** - * If a push to any URI returned by {@link #select(List)} fails, {@link #failover(List, - * URIish)}} is invoked to select the next URI for retry. + * If a push to any URI returned by {@link #select(Project.NameKey, List)} fails, {@link + * #failover(List, URIish)}} is invoked to select the next URI for retry. */ default URIish failover(List<URIish> candidates, URIish failed) { return failed;
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md index e8bf92a..014ca15 100644 --- a/src/main/resources/Documentation/config.md +++ b/src/main/resources/Documentation/config.md
@@ -768,6 +768,30 @@ unreachable host does not block replication. Has no effect if only one URL is configured. + `projectSharded` + : Push to one URL, chosen so that a given project always maps to the same + URL. Like `roundRobin` each push is written exactly once, and load is + spread across the URLs, but consecutive updates for one project always + go to the same URL. + + Prefer this over `roundRobin` when replica hosts share a single backend. + Replication tasks are coalesced per (project, URL), so rotating URLs + makes successive updates for one project run as separate tasks that race + against the same backend; pinning a project to one URL collapses them + into a single task and keeps the receiving host's caches warm. + + The mapping is computed from a hash of the project name over the sorted + list of URLs, so it does not depend on the order in which the URLs are + configured and is identical on every host reading the same config. + Adding or removing a URL remaps projects across the remaining URLs. + + On a transport error during retry, the next push attempt fails over to a + different URL in the rotation (bounded by `replicationRetry`), so a single + unreachable host does not block replication. Has no effect if only one URL + is configured. + + Has no effect if only one URL is configured. + Defaults to `all`. Directory `replication`
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/DestinationConfigurationTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/DestinationConfigurationTest.java index d0beea1..e178ab1 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/DestinationConfigurationTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/DestinationConfigurationTest.java
@@ -205,4 +205,16 @@ assertThat(objectUnderTest.getUrlDistributionStrategy()) .isEqualTo(UrlDistributionStrategy.ROUND_ROBIN); } + + @Test + public void shouldSetUrlDistributionToProjectShardedWhenConfigured() { + // given + when(cfgMock.getString("remote", REMOTE, "urlDistributionStrategy")) + .thenReturn("projectSharded"); + objectUnderTest = new DestinationConfiguration(remoteConfigMock, cfgMock); + + // when / then + assertThat(objectUnderTest.getUrlDistributionStrategy()) + .isEqualTo(UrlDistributionStrategy.PROJECT_SHARDED); + } }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java index 523563a..3e9e825 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java
@@ -650,6 +650,84 @@ } @Test + public void shouldReplicateToOnlyOneUrlWhenProjectShardedEnabled() throws Exception { + Project.NameKey replica1Project = createTestProject(project + "replica1"); + Project.NameKey replica2Project = createTestProject(project + "replica2"); + + setReplicationDestination( + "foo", List.of("replica1", "replica2"), ALL_PROJECTS, TEST_REPLICATION_DELAY_SECONDS); + setUrlDistribution("foo", UrlDistributionStrategy.PROJECT_SHARDED); + reloadConfig(); + + String newRef = "refs/heads/newForTest"; + createNewBranchWithoutPush("refs/heads/master", newRef); + + plugin + .getSysInjector() + .getInstance(ReplicationQueue.class) + .scheduleFullSync( + project, null, PushOne.ALL_REFS, Set.of(), new ReplicationState(NO_OP), true); + + // Wait for the push to land in at least one replica + try (Repository r1 = repoManager.openRepository(replica1Project); + Repository r2 = repoManager.openRepository(replica2Project)) { + waitUntil(() -> checkedGetRef(r1, newRef) != null || checkedGetRef(r2, newRef) != null); + + // Exactly one replica should have received the push + boolean r1HasRef = checkedGetRef(r1, newRef) != null; + boolean r2HasRef = checkedGetRef(r2, newRef) != null; + assertThat(r1HasRef ^ r2HasRef).isTrue(); + } + } + + @Test + public void shouldReuseSameUrlOnConsecutivePushesWhenProjectShardedEnabled() throws Exception { + Project.NameKey replica1Project = createTestProject(project + "replica1"); + Project.NameKey replica2Project = createTestProject(project + "replica2"); + + setReplicationDestination( + "foo", List.of("replica1", "replica2"), ALL_PROJECTS, TEST_REPLICATION_DELAY_SECONDS); + setUrlDistribution("foo", UrlDistributionStrategy.PROJECT_SHARDED); + reloadConfig(); + + String branch1 = "refs/heads/branch1"; + String branch2 = "refs/heads/branch2"; + createNewBranchWithoutPush("refs/heads/master", branch1); + + ReplicationQueue queue = plugin.getSysInjector().getInstance(ReplicationQueue.class); + + queue.scheduleFullSync( + project, null, PushOne.ALL_REFS, Set.of(), new ReplicationState(NO_OP), true); + + // The project is pinned to one of the two replicas; discover which one. + Project.NameKey pinnedProject; + Project.NameKey otherProject; + try (Repository r1 = repoManager.openRepository(replica1Project); + Repository r2 = repoManager.openRepository(replica2Project)) { + waitUntil(() -> checkedGetRef(r1, branch1) != null || checkedGetRef(r2, branch1) != null); + + boolean replica1IsPinned = checkedGetRef(r1, branch1) != null; + assertThat(replica1IsPinned ^ (checkedGetRef(r2, branch1) != null)).isTrue(); + pinnedProject = replica1IsPinned ? replica1Project : replica2Project; + otherProject = replica1IsPinned ? replica2Project : replica1Project; + } + + // Second sync must land on the same replica rather than rotating to the other one + createNewBranchWithoutPush("refs/heads/master", branch2); + queue.scheduleFullSync( + project, null, PushOne.ALL_REFS, Set.of(), new ReplicationState(NO_OP), true); + + try (Repository pinnedRepo = repoManager.openRepository(pinnedProject)) { + waitUntil(() -> checkedGetRef(pinnedRepo, branch2) != null); + } + + try (Repository otherRepo = repoManager.openRepository(otherProject)) { + assertThat(checkedGetRef(otherRepo, branch1)).isNull(); + assertThat(checkedGetRef(otherRepo, branch2)).isNull(); + } + } + + @Test public void shouldReplicateToMatchingRemote() throws Exception { Project.NameKey targetProject = createTestProject(project + "replica");
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/UrlDistributionStrategyTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/UrlDistributionStrategyTest.java new file mode 100644 index 0000000..2ded31e --- /dev/null +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/UrlDistributionStrategyTest.java
@@ -0,0 +1,170 @@ +// Copyright (C) 2026 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.googlesource.gerrit.plugins.replication; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import com.google.gerrit.entities.Project; +import java.net.URISyntaxException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Random; +import java.util.Set; +import org.eclipse.jgit.transport.URIish; +import org.junit.Test; + +public class UrlDistributionStrategyTest { + private static final Project.NameKey PROJECT = Project.nameKey("some/project"); + private static final Project.NameKey OTHER_PROJECT = Project.nameKey("some/other/project"); + + private static ImmutableList<URIish> uris(String... hosts) throws URISyntaxException { + ImmutableList.Builder<URIish> uris = ImmutableList.builder(); + for (String host : hosts) { + uris.add(new URIish("git://" + host + "/${name}.git")); + } + return uris.build(); + } + + @Test + public void shouldResolveConfigKeys() { + assertThat(UrlDistributionStrategy.fromConfig("all")).isEqualTo(UrlDistributionStrategy.ALL); + assertThat(UrlDistributionStrategy.fromConfig("roundRobin")) + .isEqualTo(UrlDistributionStrategy.ROUND_ROBIN); + assertThat(UrlDistributionStrategy.fromConfig("projectSharded")) + .isEqualTo(UrlDistributionStrategy.PROJECT_SHARDED); + } + + @Test + public void shouldFallBackToAllForUnknownConfigKey() { + assertThat(UrlDistributionStrategy.fromConfig("bogus")).isEqualTo(UrlDistributionStrategy.ALL); + assertThat(UrlDistributionStrategy.fromConfig(null)).isEqualTo(UrlDistributionStrategy.ALL); + } + + @Test + public void allShouldSelectEveryCandidate() throws Exception { + ImmutableList<URIish> candidates = uris("replica1", "replica2", "replica3"); + UrlDistributionStrategy.Instance distributor = UrlDistributionStrategy.ALL.newInstance(); + + assertThat(distributor.select(PROJECT, candidates)).isEqualTo(candidates); + } + + @Test + public void roundRobinShouldRotateOnConsecutiveSelections() throws Exception { + ImmutableList<URIish> candidates = uris("replica1", "replica2"); + UrlDistributionStrategy.Instance distributor = + UrlDistributionStrategy.ROUND_ROBIN.newInstance(); + + assertThat(distributor.select(PROJECT, candidates)).containsExactly(candidates.get(0)); + assertThat(distributor.select(PROJECT, candidates)).containsExactly(candidates.get(1)); + assertThat(distributor.select(PROJECT, candidates)).containsExactly(candidates.get(0)); + } + + @Test + public void roundRobinShouldRotateRegardlessOfProject() throws Exception { + ImmutableList<URIish> candidates = uris("replica1", "replica2"); + UrlDistributionStrategy.Instance distributor = + UrlDistributionStrategy.ROUND_ROBIN.newInstance(); + + assertThat(distributor.select(PROJECT, candidates)).containsExactly(candidates.get(0)); + assertThat(distributor.select(OTHER_PROJECT, candidates)).containsExactly(candidates.get(1)); + } + + @Test + public void projectShardedShouldSelectExactlyOneCandidate() throws Exception { + ImmutableList<URIish> candidates = uris("replica1", "replica2", "replica3"); + UrlDistributionStrategy.Instance distributor = + UrlDistributionStrategy.PROJECT_SHARDED.newInstance(); + + List<URIish> selected = distributor.select(PROJECT, candidates); + + assertThat(selected).hasSize(1); + assertThat(candidates).containsAtLeastElementsIn(selected); + } + + @Test + public void projectShardedShouldPinProjectToSameCandidateAcrossSelections() throws Exception { + ImmutableList<URIish> candidates = uris("replica1", "replica2", "replica3"); + UrlDistributionStrategy.Instance distributor = + UrlDistributionStrategy.PROJECT_SHARDED.newInstance(); + + List<URIish> first = distributor.select(PROJECT, candidates); + for (int i = 0; i < 10; i++) { + assertThat(distributor.select(PROJECT, candidates)).isEqualTo(first); + } + } + + @Test + public void projectShardedShouldAgreeAcrossInstances() throws Exception { + ImmutableList<URIish> candidates = uris("replica1", "replica2", "replica3"); + + List<URIish> first = + UrlDistributionStrategy.PROJECT_SHARDED.newInstance().select(PROJECT, candidates); + List<URIish> second = + UrlDistributionStrategy.PROJECT_SHARDED.newInstance().select(PROJECT, candidates); + + assertThat(second).isEqualTo(first); + } + + @Test + public void projectShardedShouldIgnoreCandidateOrdering() throws Exception { + ImmutableList<URIish> candidates = + uris("replica1", "replica2", "replica3", "replica4", "replica5"); + UrlDistributionStrategy.Instance distributor = + UrlDistributionStrategy.PROJECT_SHARDED.newInstance(); + + List<URIish> expected = distributor.select(PROJECT, candidates); + + List<URIish> shuffled = new ArrayList<>(candidates); + Random random = new Random(42); + for (int i = 0; i < 20; i++) { + Collections.shuffle(shuffled, random); + assertThat(distributor.select(PROJECT, shuffled)).isEqualTo(expected); + } + } + + @Test + public void projectShardedShouldSpreadProjectsOverAllCandidates() throws Exception { + ImmutableList<URIish> candidates = uris("replica1", "replica2", "replica3"); + UrlDistributionStrategy.Instance distributor = + UrlDistributionStrategy.PROJECT_SHARDED.newInstance(); + + Set<URIish> selected = new HashSet<>(); + for (int i = 0; i < 100; i++) { + selected.addAll(distributor.select(Project.nameKey("project" + i), candidates)); + } + + assertThat(selected).containsExactlyElementsIn(candidates); + } + + @Test + public void projectShardedShouldSelectOnlyCandidateWhenSingleUrlConfigured() throws Exception { + ImmutableList<URIish> candidates = uris("replica1"); + UrlDistributionStrategy.Instance distributor = + UrlDistributionStrategy.PROJECT_SHARDED.newInstance(); + + assertThat(distributor.select(PROJECT, candidates)).isEqualTo(candidates); + assertThat(distributor.select(OTHER_PROJECT, candidates)).isEqualTo(candidates); + } + + @Test + public void shouldSelectNothingWhenThereAreNoCandidates() { + for (UrlDistributionStrategy strategy : UrlDistributionStrategy.values()) { + assertThat(strategy.newInstance().select(PROJECT, List.of())).isEmpty(); + } + } +}