Merge branch 'stable-3.6' into stable-3.7 * stable-3.6: Log stacktrace when replication fails Change-Id: I5473529fc1e6dcf73994e0cc20872c007210439a
diff --git a/example-setup/broker/Dockerfile b/example-setup/broker/Dockerfile index b79470c..67eecd9 100644 --- a/example-setup/broker/Dockerfile +++ b/example-setup/broker/Dockerfile
@@ -1,4 +1,4 @@ -FROM gerritcodereview/gerrit:3.6.3-almalinux8 +FROM gerritcodereview/gerrit:3.7.2-almalinux8 USER root
diff --git a/example-setup/http/Dockerfile b/example-setup/http/Dockerfile index 77fed72..afadb4f 100644 --- a/example-setup/http/Dockerfile +++ b/example-setup/http/Dockerfile
@@ -1,4 +1,4 @@ -FROM gerritcodereview/gerrit:3.6.3-almalinux8 +FROM gerritcodereview/gerrit:3.7.2-almalinux8 USER root
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueue.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueue.java index ef95596..cdef514 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueue.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueue.java
@@ -14,6 +14,8 @@ package com.googlesource.gerrit.plugins.replication.pull; +import static java.util.concurrent.TimeUnit.MILLISECONDS; + import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; import com.google.common.collect.Queues; @@ -38,6 +40,7 @@ import com.googlesource.gerrit.plugins.replication.pull.api.data.RevisionData; import com.googlesource.gerrit.plugins.replication.pull.api.exception.MissingParentObjectException; import com.googlesource.gerrit.plugins.replication.pull.client.FetchApiClient; +import com.googlesource.gerrit.plugins.replication.pull.client.FetchRestApiClient; import com.googlesource.gerrit.plugins.replication.pull.client.HttpResult; import com.googlesource.gerrit.plugins.replication.pull.filter.ApplyObjectsRefsFilter; import com.googlesource.gerrit.plugins.replication.pull.filter.ExcludedRefsFilter; @@ -288,7 +291,7 @@ } if (!callSuccessful) { - callFetch(source, project, refName, state); + callFetch(source, project, refName, state, FetchRestApiClient.FORCE_ASYNC); } }; } @@ -334,7 +337,7 @@ state); } - return (source) -> callFetch(source, project, refName, state); + return (source) -> callFetch(source, project, refName, state, false); } private boolean callSendObject( @@ -461,7 +464,11 @@ } private boolean callFetch( - Source source, Project.NameKey project, String refName, ReplicationState state) { + Source source, + Project.NameKey project, + String refName, + ReplicationState state, + boolean forceAsyncCall) { boolean resultIsSuccessful = true; if (source.wouldFetchProject(project) && source.wouldFetchRef(refName)) { for (String apiUrl : source.getApis()) { @@ -470,7 +477,13 @@ FetchApiClient fetchClient = fetchClientFactory.create(source); repLog.info("Pull replication REST API fetch to {} for {}:{}", apiUrl, project, refName); Context<String> timer = fetchMetrics.startEnd2End(source.getRemoteConfigName()); - HttpResult result = fetchClient.callFetch(project, refName, uri); + HttpResult result = + fetchClient.callFetch( + project, + refName, + uri, + MILLISECONDS.toNanos(System.currentTimeMillis()), + forceAsyncCall); long elapsedMs = TimeUnit.NANOSECONDS.toMillis(timer.stop()); boolean resultSuccessful = result.isSuccessful(); repLog.info(
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueueMetrics.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueueMetrics.java index b0fa7e9..abb01b7 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueueMetrics.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueueMetrics.java
@@ -35,6 +35,7 @@ public class ReplicationQueueMetrics { private static final String EVENTS = "events"; private static final String TASKS = "tasks"; + private static final String REFS = "refs"; public static final String REPLICATION_QUEUE_METRICS = "ReplicationQueueMetrics"; private final Counter1<String> tasksScheduled; @@ -52,6 +53,10 @@ private final Counter1<String> tasksStarted; private final Set<RegistrationHandle> metricsHandles; + private final Counter1<String> refsFetchStarted; + private final Counter1<String> refsFetchCompleted; + private final Counter1<String> refsFetchFailed; + public class RunnableWithMetrics implements Runnable { private final Source source; private final Runnable runnable; @@ -64,13 +69,17 @@ @Override public void run() { incrementTaskStarted(source); + incrementFetchRefsStarted(source, runnable); + runnable.run(); if (runnable instanceof Completable) { Completable completedRunnable = (Completable) runnable; if (completedRunnable.hasSucceeded()) { incrementTaskCompleted(source); + incrementFetchRefsCompleted(source, runnable); } else { incrementTaskFailed(source); + incrementFetchRefsFailed(source, runnable); } } } @@ -170,6 +179,31 @@ .setUnit(TASKS), sourceField)); + refsFetchStarted = + registerMetric( + metricMaker.newCounter( + "fetch/refs/started", + new Description("Refs for which fetch operation have started") + .setCumulative() + .setUnit(REFS), + sourceField)); + refsFetchCompleted = + registerMetric( + metricMaker.newCounter( + "fetch/refs/completed", + new Description("Refs for which fetch operation have completed") + .setCumulative() + .setUnit(REFS), + sourceField)); + refsFetchFailed = + registerMetric( + metricMaker.newCounter( + "fetch/refs/failed", + new Description("Refs for which fetch operation have failed") + .setCumulative() + .setUnit(REFS), + sourceField)); + this.metricMaker = metricMaker; } @@ -269,6 +303,25 @@ tasksStarted.increment(source.getRemoteConfigName()); } + public void incrementFetchRefsStarted(Source source, Runnable runnableTask) { + incrementFetchRefsCounter(source, runnableTask, refsFetchStarted); + } + + public void incrementFetchRefsCompleted(Source source, Runnable runnableTask) { + incrementFetchRefsCounter(source, runnableTask, refsFetchCompleted); + } + + public void incrementFetchRefsFailed(Source source, Runnable runnableTask) { + incrementFetchRefsCounter(source, runnableTask, refsFetchFailed); + } + + private void incrementFetchRefsCounter( + Source source, Runnable runnableTask, Counter1<String> counter) { + if (runnableTask instanceof FetchOne) { + counter.incrementBy(source.getRemoteConfigName(), ((FetchOne) runnableTask).getRefs().size()); + } + } + public Runnable runWithMetrics(Source source, Runnable runnableTask) { if (runnableTask instanceof RunnableWithMetrics) { return runnableTask;
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/Source.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/Source.java index 012a046..74a6be1 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/Source.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/Source.java
@@ -678,6 +678,7 @@ : RefUpdate.Result.REJECTED_OTHER_REASON; postReplicationFailedEvent(fetchOp, trackingRefUpdate); queueMetrics.incrementTaskFailed(this); + queueMetrics.incrementFetchRefsFailed(this, fetchOp); if (fetchOp.setToRetry()) { postReplicationScheduledEvent(fetchOp);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/SourceConfiguration.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/SourceConfiguration.java index 0e840bd..b6647bd 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/SourceConfiguration.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/SourceConfiguration.java
@@ -16,6 +16,7 @@ import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableList; +import com.google.common.flogger.FluentLogger; import com.google.gerrit.server.config.ConfigUtil; import com.googlesource.gerrit.plugins.replication.RemoteConfiguration; import java.util.concurrent.TimeUnit; @@ -23,6 +24,7 @@ import org.eclipse.jgit.transport.RemoteConfig; public class SourceConfiguration implements RemoteConfiguration { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); static final int DEFAULT_REPLICATION_DELAY = 4; static final int DEFAULT_RESCHEDULE_DELAY = 3; static final int DEFAULT_SLOW_LATENCY_THRESHOLD_SECS = 900; @@ -64,6 +66,13 @@ apis = ImmutableList.copyOf(cfg.getStringList("remote", name, "apiUrl")); connectionTimeout = cfg.getInt("remote", name, "connectionTimeout", DEFAULT_CONNECTION_TIMEOUT_MS); + int connectionTimeoutInSec = connectionTimeout / 1000; + if (connectionTimeoutInSec < getRemoteConfig().getTimeout()) { + logger.atWarning().log( + "The connection timeout is currently set to %s sec, which is less than the timeout value of %s sec. " + + "To avoid potential issues, consider increasing the connection timeout to exceed the timeout value.", + connectionTimeoutInSec, getRemoteConfig().getTimeout()); + } idleTimeout = cfg.getInt("remote", name, "idleTimeout", DEFAULT_MAX_CONNECTION_INACTIVITY_MS); maxConnectionsPerRoute = cfg.getInt("replication", "maxConnectionsPerRoute", DEFAULT_CONNECTIONS_PER_ROUTE);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/PullReplicationFilter.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/PullReplicationFilter.java index d0e5c10..f501e90 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/PullReplicationFilter.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/PullReplicationFilter.java
@@ -145,7 +145,7 @@ } catch (AuthException e) { RestApiServlet.replyError( httpRequest, httpResponse, SC_FORBIDDEN, e.getMessage(), e.caching(), e); - } catch (MalformedJsonException | JsonParseException e) { + } catch (MalformedJsonException | JsonParseException | IllegalArgumentException e) { logger.atFine().withCause(e).log("REST call failed on JSON parsing"); RestApiServlet.replyError( httpRequest, httpResponse, SC_BAD_REQUEST, "Invalid json in request", e);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchApiClient.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchApiClient.java index f7ed4cb..38d5d57 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchApiClient.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchApiClient.java
@@ -32,12 +32,17 @@ } HttpResult callFetch( - Project.NameKey project, String refName, URIish targetUri, long startTimeNanos) + Project.NameKey project, + String refName, + URIish targetUri, + long startTimeNanos, + boolean forceAsyncCall) throws ClientProtocolException, IOException; default HttpResult callFetch(Project.NameKey project, String refName, URIish targetUri) throws ClientProtocolException, IOException { - return callFetch(project, refName, targetUri, MILLISECONDS.toNanos(System.currentTimeMillis())); + return callFetch( + project, refName, targetUri, MILLISECONDS.toNanos(System.currentTimeMillis()), false); } /**
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClient.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClient.java index 7607e4b..5579415 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClient.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClient.java
@@ -63,6 +63,8 @@ import org.eclipse.jgit.transport.URIish; public class FetchRestApiClient implements FetchApiClient, ResponseHandler<HttpResult> { + public static final boolean FORCE_ASYNC = true; + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); static String GERRIT_ADMIN_PROTOCOL_PREFIX = "gerrit+"; @@ -107,15 +109,31 @@ this.urlAuthenticationPrefix = bearerTokenProvider.get().map(br -> "").orElse("a/"); } - /* (non-Javadoc) - * @see com.googlesource.gerrit.plugins.replication.pull.client.FetchApiClient#callFetch(com.google.gerrit.entities.Project.NameKey, java.lang.String, org.eclipse.jgit.transport.URIish) - */ @Override public HttpResult callFetch( - Project.NameKey project, String refName, URIish targetUri, long startTimeNanos) + NameKey project, + String refName, + URIish targetUri, + long startTimeNanos, + boolean forceAsyncFetch) + throws ClientProtocolException, IOException { + return doCallFetch( + project, + refName, + targetUri, + startTimeNanos, + forceAsyncFetch || !syncRefsFilter.match(refName)); + } + + private HttpResult doCallFetch( + Project.NameKey project, + String refName, + URIish targetUri, + long startTimeNanos, + boolean callAsync) throws IOException { String url = formatUrl(targetUri.toString(), project, "fetch"); - Boolean callAsync = !syncRefsFilter.match(refName); + HttpPost post = new HttpPost(url); post.setEntity( new StringEntity(
diff --git a/src/main/resources/Documentation/metrics.md b/src/main/resources/Documentation/metrics.md index ac1539c..76861d4 100644 --- a/src/main/resources/Documentation/metrics.md +++ b/src/main/resources/Documentation/metrics.md
@@ -63,6 +63,22 @@ - `failed_max_retries`: (counter) number of tasks that have reached their maximum retry count but never succeeded. +### plugins/@PLUGIN@/fetch/refs/<metric>/<source> + +Cumulative number of refs included in the Git fetch operation. + +The `<metric>` field can have one of the values described here below, +while the `<source>` represents the replication source endpoint name. + +- `started`: (counter) number of refs for which fetch operation have started. + +- `completed`: (counter) number of refs for which fetch operation have completed. + +- `failed`: (counter) number of refs for which fetch operation have failed. + +> Bear in mind that the fetch with special `..all..` ref spec will be counted +> as 1 even if the fetch cause multiple refs to be fetched. + ### plugins/@PLUGIN@ - `apply_object_latency`: (timer) execution time statistics for the
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueueTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueueTest.java index a07aa55..3d4d531 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueueTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueueTest.java
@@ -61,6 +61,7 @@ import java.util.Optional; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.storage.file.FileBasedConfig; +import org.eclipse.jgit.transport.URIish; import org.eclipse.jgit.util.FS; import org.junit.Before; import org.junit.Test; @@ -152,7 +153,6 @@ .thenReturn(successfulHttpResult); when(successfulHttpResult.isSuccessful()).thenReturn(true); when(httpResult.isSuccessful()).thenReturn(true); - when(fetchHttpResult.isSuccessful()).thenReturn(true); when(httpResult.isProjectMissing(any())).thenReturn(false); when(applyObjectsRefsFilter.match(any())).thenReturn(false); @@ -250,31 +250,31 @@ @Test public void shouldFallbackToCallFetchWhenIOException() throws Exception { - Event event = new TestEvent("refs/changes/01/1/meta"); + RefUpdatedEvent event = new TestEvent("refs/changes/01/1/meta"); objectUnderTest.start(); when(revReader.read(any(), any(), anyString(), anyInt())).thenThrow(IOException.class); objectUnderTest.onEvent(event); - verify(fetchRestApiClient).callFetch(any(), anyString(), any()); + verifyFallbackToRestApiClientFetchAsync(event); } @Test public void shouldFallbackToCallFetchWhenLargeRef() throws Exception { - Event event = new TestEvent("refs/changes/01/1/1"); + RefUpdatedEvent event = new TestEvent("refs/changes/01/1/1"); objectUnderTest.start(); when(revReader.read(any(), any(), anyString(), anyInt())).thenReturn(Optional.empty()); objectUnderTest.onEvent(event); - verify(fetchRestApiClient).callFetch(any(), anyString(), any()); + verifyFallbackToRestApiClientFetchAsync(event); } @Test public void shouldFallbackToCallFetchWhenParentObjectIsMissing() throws Exception { - Event event = new TestEvent("refs/changes/01/1/1"); + RefUpdatedEvent event = new TestEvent("refs/changes/01/1/1"); objectUnderTest.start(); when(httpResult.isSuccessful()).thenReturn(false); @@ -284,7 +284,7 @@ objectUnderTest.onEvent(event); - verify(fetchRestApiClient).callFetch(any(), anyString(), any()); + verifyFallbackToRestApiClientFetchAsync(event); } @Test @@ -486,4 +486,14 @@ return projectName; } } + + private void verifyFallbackToRestApiClientFetchAsync(RefUpdatedEvent event) throws IOException { + verify(fetchRestApiClient) + .callFetch( + eq(event.getProjectNameKey()), + eq(event.getRefName()), + any(URIish.class), + any(Long.class), + eq(FetchRestApiClient.FORCE_ASYNC)); + } }