Allow replicationRetry to be configured with a time-unit suffix remote.NAME.replicationRetry was parsed as a bare integer and scheduled in TimeUnit.MINUTES, so the smallest non-zero offline-retry backoff an admin could configure was a full minute. That minutes-only quantum also prevents ReplicationIT from driving a sub-minute retry, which keeps its two new-project cases slow. Parse replicationRetry with ConfigUtil.getTimeUnit and schedule the retry in seconds. A value without a unit keeps its historical meaning of minutes, so existing configurations are unchanged, while a value with a time-unit suffix -- e.g. "30 s", "90 s" or "2 m" -- is honoured as written. getRetryDelay() now returns seconds, matching the RemoteConfiguration javadoc that already documented it as such, and both reschedule sites in Destination (the direct retry and the failover replacement) schedule in TimeUnit.SECONDS instead of TimeUnit.MINUTES. This follows the httpd.maxwait precedent in gerrit-core, which likewise defaults a bare number to minutes and accepts an optional unit suffix. Use the new option to speed up the two new-project cases (shouldReplicateNewProjectWithoutRefLog and shouldCreateNewProjectWithRefLog): their first ref-push finds the replica repository missing, PushOne#createRepository creates it, and the push is rescheduled as REPOSITORY_MISSING, which waited the one-minute replicationRetry. They now set replicationRetry to TEST_REPLICATION_RETRY_SECONDS (1s) so the missing-repository retry fires in a second, with TEST_NEW_PROJECT_TIMEOUT tightened to match. The shared TEST_REPLICATION_RETRY_MINUTES is left untouched so tests that rely on the real one-minute retry (e.g. ReplicationStorageIT) keep their timing. The two cases drop from ~60s to ~4s; ReplicationIT drops from ~173s to ~61s, all 26 green. Release-Notes: remote.NAME.replicationRetry now accepts an optional time-unit suffix (e.g. "30 s"); a bare number still means minutes. Change-Id: I54f8e3d48d20568b2858abde1e1b06f35bf27841
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 08a6c62..c1a1936 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java
@@ -581,7 +581,7 @@ * <p>If the reason for rescheduling is to avoid a collision with an in-flight push to the same * URI, we don't mark the operation as "retrying," and we schedule using the replication delay, * rather than the retry delay. Otherwise, the operation is marked as "retrying" and scheduled to - * run following the minutes count determined by class attribute retryDelay. + * run following the retry delay (in seconds) determined by class attribute retryDelay. * * <p>In case the PushOp instance to be scheduled has same URI than one marked as "retrying," it * adds to the one pending the refs list of the parameter instance. @@ -673,7 +673,7 @@ replicationTasksStorage.get().reset(pushOp); @SuppressWarnings("unused") ScheduledFuture<?> ignored2 = - pool.schedule(pushOp, config.getRetryDelay(), TimeUnit.MINUTES); + pool.schedule(pushOp, config.getRetryDelay(), TimeUnit.SECONDS); } } else { pushOp.canceledByReplication(); @@ -725,7 +725,7 @@ queue.pending.put(newUri, replacement); @SuppressWarnings("unused") ScheduledFuture<?> ignored = - pool.schedule(replacement, config.getRetryDelay(), TimeUnit.MINUTES); + pool.schedule(replacement, config.getRetryDelay(), TimeUnit.SECONDS); return true; }); repLog.atInfo().log(
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationConfiguration.java b/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationConfiguration.java index 03ba914..02389d6 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationConfiguration.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationConfiguration.java
@@ -32,6 +32,7 @@ public class DestinationConfiguration implements RemoteConfiguration { static final int DEFAULT_REPLICATION_DELAY = 15; static final int DEFAULT_RESCHEDULE_DELAY = 3; + static final int DEFAULT_REPLICATION_RETRY_MINUTES = 1; static final int DEFAULT_DRAIN_QUEUE_ATTEMPTS = 0; private static final int DEFAULT_SLOW_LATENCY_THRESHOLD_SECS = 900; @@ -73,7 +74,7 @@ projects = ImmutableList.copyOf(cfg.getStringList("remote", name, "projects")); excludeProjects = ImmutableList.copyOf(cfg.getStringList("remote", name, "excludeProjects")); adminUrls = ImmutableList.copyOf(cfg.getStringList("remote", name, "adminUrl")); - retryDelay = Math.max(0, getInt(remoteConfig, cfg, "replicationretry", 1)); + retryDelay = getRetryDelaySeconds(remoteConfig, cfg); drainQueueAttempts = Math.max(0, getInt(remoteConfig, cfg, "drainQueueAttempts", DEFAULT_DRAIN_QUEUE_ATTEMPTS)); poolThreads = Math.max(0, getInt(remoteConfig, cfg, "threads", 1)); @@ -231,6 +232,33 @@ return cfg.getInt("remote", rc.getName(), name, defValue); } + /** + * Parses {@code remote.NAME.replicationRetry} into seconds. For backwards compatibility a value + * without a time-unit suffix keeps its historical meaning of minutes -- including a negative bare + * number, which the historical {@code cfg.getInt(...)} parsing also accepted and which is clamped + * to zero below -- so existing configurations are unchanged. A value with a time-unit suffix -- + * e.g. {@code 30 s}, {@code 90 s} or {@code 2 m} -- is honoured as written, letting an admin + * configure a sub-minute offline-retry backoff. The result is clamped to {@code [0, + * Integer.MAX_VALUE]} so it never overflows the {@code int} the scheduler expects. + */ + private static int getRetryDelaySeconds(RemoteConfig rc, Config cfg) { + String value = cfg.getString("remote", rc.getName(), "replicationRetry"); + long defaultSeconds = TimeUnit.MINUTES.toSeconds(DEFAULT_REPLICATION_RETRY_MINUTES); + long seconds; + if (value == null || value.trim().isEmpty()) { + seconds = defaultSeconds; + } else if (value.trim().matches("-?[0-9]+")) { + seconds = TimeUnit.MINUTES.toSeconds(Long.parseLong(value.trim())); + } else { + // Use the config overload so an invalid value is reported against + // remote.NAME.replicationRetry rather than just the raw string. + seconds = + ConfigUtil.getTimeUnit( + cfg, "remote", rc.getName(), "replicationRetry", defaultSeconds, TimeUnit.SECONDS); + } + return (int) Math.max(0, Math.min(seconds, Integer.MAX_VALUE)); + } + @Override public int getSlowLatencyThreshold() { return slowLatencyThreshold;
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md index a2f5459..e8bf92a 100644 --- a/src/main/resources/Documentation/config.md +++ b/src/main/resources/Documentation/config.md
@@ -501,6 +501,11 @@ This is a Gerrit specific extension to the Git remote block. + For backwards compatibility, a bare number is interpreted as + minutes. A time-unit suffix may be appended to configure a + finer granularity, for example `30 s`, `90 s` or `2 m`; the value is + applied at second precision. + By default, 1 minute. remote.NAME.replicationMaxRetries
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 c7b4174..d0beea1 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/DestinationConfigurationTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/DestinationConfigurationTest.java
@@ -15,6 +15,7 @@ package com.googlesource.gerrit.plugins.replication; import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; import static org.mockito.Mockito.when; import org.eclipse.jgit.lib.Config; @@ -100,6 +101,96 @@ } @Test + public void shouldDefaultReplicationRetryToOneMinute() { + assertThat(objectUnderTest.getRetryDelay()).isEqualTo(60); + } + + @Test + public void shouldTreatBareReplicationRetryAsMinutes() { + // given + when(cfgMock.getString("remote", REMOTE, "replicationRetry")).thenReturn("2"); + objectUnderTest = new DestinationConfiguration(remoteConfigMock, cfgMock); + + // when / then + assertThat(objectUnderTest.getRetryDelay()).isEqualTo(120); + } + + @Test + public void shouldParseReplicationRetryWithSecondsSuffix() { + // given + when(cfgMock.getString("remote", REMOTE, "replicationRetry")).thenReturn("30 s"); + objectUnderTest = new DestinationConfiguration(remoteConfigMock, cfgMock); + + // when / then + assertThat(objectUnderTest.getRetryDelay()).isEqualTo(30); + } + + @Test + public void shouldParseReplicationRetryWithMinutesSuffix() { + // given + when(cfgMock.getString("remote", REMOTE, "replicationRetry")).thenReturn("2 m"); + objectUnderTest = new DestinationConfiguration(remoteConfigMock, cfgMock); + + // when / then + assertThat(objectUnderTest.getRetryDelay()).isEqualTo(120); + } + + @Test + public void shouldTreatZeroReplicationRetryAsNoDelay() { + // given + when(cfgMock.getString("remote", REMOTE, "replicationRetry")).thenReturn("0"); + objectUnderTest = new DestinationConfiguration(remoteConfigMock, cfgMock); + + // when / then + assertThat(objectUnderTest.getRetryDelay()).isEqualTo(0); + } + + @Test + public void shouldClampNegativeBareReplicationRetryToZero() { + // given: a bare negative was accepted historically (cfg.getInt) and clamped to zero + when(cfgMock.getString("remote", REMOTE, "replicationRetry")).thenReturn("-1"); + objectUnderTest = new DestinationConfiguration(remoteConfigMock, cfgMock); + + // when / then + assertThat(objectUnderTest.getRetryDelay()).isEqualTo(0); + } + + @Test + public void shouldDefaultReplicationRetryWhenEmpty() { + // given + when(cfgMock.getString("remote", REMOTE, "replicationRetry")).thenReturn(" "); + objectUnderTest = new DestinationConfiguration(remoteConfigMock, cfgMock); + + // when / then + assertThat(objectUnderTest.getRetryDelay()).isEqualTo(60); + } + + @Test + public void shouldClampHugeReplicationRetryToIntMax() { + // given: a value whose seconds exceed Integer.MAX_VALUE must not overflow to a negative int + when(cfgMock.getString("remote", REMOTE, "replicationRetry")).thenReturn("999999999999 s"); + objectUnderTest = new DestinationConfiguration(remoteConfigMock, cfgMock); + + // when / then + assertThat(objectUnderTest.getRetryDelay()).isEqualTo(Integer.MAX_VALUE); + } + + @Test + public void shouldRejectInvalidReplicationRetry() { + // given + when(cfgMock.getString("remote", REMOTE, "replicationRetry")).thenReturn("banana"); + + // when + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, + () -> new DestinationConfiguration(remoteConfigMock, cfgMock)); + + // then: the error names the offending config key + assertThat(thrown).hasMessageThat().contains("remote." + REMOTE + ".replicationRetry"); + } + + @Test public void shouldDefaultUrlDistributionToAll() { assertThat(objectUnderTest.getUrlDistributionStrategy()).isEqualTo(UrlDistributionStrategy.ALL); }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationDaemon.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationDaemon.java index 436fb1c..5764521 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationDaemon.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationDaemon.java
@@ -56,15 +56,23 @@ protected static final int TEST_REPLICATION_DELAY_SECONDS = 1; protected static final int TEST_LONG_REPLICATION_DELAY_SECONDS = 30; protected static final int TEST_REPLICATION_RETRY_MINUTES = 1; + // Sub-minute replicationRetry used by the new-project cases so their missing-repository retry + // fires in a second instead of waiting the full minute (see remote.NAME.replicationRetry). + protected static final int TEST_REPLICATION_RETRY_SECONDS = 1; protected static final int TEST_PUSH_TIME_SECONDS = 1; protected static final int TEST_PROJECT_CREATION_SECONDS = 10; protected static final Duration TEST_PUSH_TIMEOUT = Duration.ofSeconds(TEST_REPLICATION_DELAY_SECONDS + TEST_PUSH_TIME_SECONDS); protected static final Duration TEST_PUSH_TIMEOUT_LONG = Duration.ofSeconds(TEST_LONG_REPLICATION_DELAY_SECONDS + TEST_PUSH_TIME_SECONDS); + // A new project's first ref-push fails as REPOSITORY_MISSING, creates the repository, and is + // retried after replicationRetry; the new-project cases set that to + // TEST_REPLICATION_RETRY_SECONDS + // (no longer a full minute), so this timeout budgets the replication delay + retry + push plus a + // cushion for project creation. protected static final Duration TEST_NEW_PROJECT_TIMEOUT = Duration.ofSeconds( - (TEST_REPLICATION_DELAY_SECONDS + TEST_REPLICATION_RETRY_MINUTES * 60) + (TEST_REPLICATION_DELAY_SECONDS + TEST_REPLICATION_RETRY_SECONDS + TEST_PUSH_TIME_SECONDS) + TEST_PROJECT_CREATION_SECONDS); @Inject private ProjectOperations projectOperations;
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 4a3bb9b..523563a 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java
@@ -87,6 +87,7 @@ @Test public void shouldReplicateNewProjectWithoutRefLog() throws Exception { setReplicationDestination("foo", "replica", ALL_PROJECTS); + setSubMinuteReplicationRetry("foo"); reloadConfig(); Project.NameKey sourceProject = createTestProject("no_reflog_project"); @@ -103,6 +104,7 @@ public void shouldCreateNewProjectWithRefLog() throws Exception { config.setBoolean("remote", "foo", "storeRefLog", true); setReplicationDestination("foo", "replica", ALL_PROJECTS); + setSubMinuteReplicationRetry("foo"); reloadConfig(); Project.NameKey sourceProject = createTestProject("reflog_project"); @@ -117,6 +119,14 @@ WaitUtil.waitUntil(() -> nonEmptyProjectExists(replicaProject), TEST_NEW_PROJECT_TIMEOUT); } + // Overrides the minute-scale replicationRetry set by setReplicationDestination with a sub-minute + // value, so the first-ref retry to a just-created project fires in seconds rather than a minute. + private void setSubMinuteReplicationRetry(String remoteName) throws IOException { + config.setString( + "remote", remoteName, "replicationRetry", TEST_REPLICATION_RETRY_SECONDS + "s"); + config.save(); + } + private static Consumer<StoredConfig> assertStoreRefLog(boolean expectedValue) { return conf -> assertThat(