From 7d987cf60dc8911e8af725f02d9493e2237c3dad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Brunner?= <stephane.brunner@camptocamp.com> Date: Thu, 2 Mar 2023 09:53:54 +0100 Subject: [PATCH 1/4] Enable Retry on exception when fetching data. --- Makefile | 2 +- core/build.gradle | 1 - .../print/http/HttpRequestFetcher.java | 44 ++++++++++++++----- ...mapfish-cli-spring-application-context.xml | 1 + .../main/resources/mapfish-spring.properties | 4 +- 5 files changed, 37 insertions(+), 15 deletions(-) diff --git a/Makefile b/Makefile index e1c9158cb8..647b7bdae1 100644 --- a/Makefile +++ b/Makefile @@ -3,7 +3,7 @@ export DOCKER_BUILDKIT = 1 .PHONY: build build: - # Required and not nesseerly exists + # Required and not necessarily exists touch CI.asc docker build $(GIT_HEAD_ARG) --target=builder --tag=mapfish_print_builder . diff --git a/core/build.gradle b/core/build.gradle index 9a1d70d825..18d6747e8f 100644 --- a/core/build.gradle +++ b/core/build.gradle @@ -132,7 +132,6 @@ dependencies { "org.springframework:spring-jdbc:5.3.27", "org.springframework:spring-tx:5.3.27", "org.springframework:spring-test:5.3.27", - "org.springframework.retry:spring-retry:1.3.4", ) metrics( "io.dropwizard.metrics:metrics-core:4.2.18", diff --git a/core/src/main/java/org/mapfish/print/http/HttpRequestFetcher.java b/core/src/main/java/org/mapfish/print/http/HttpRequestFetcher.java index 80af8047b2..ff47af31ff 100644 --- a/core/src/main/java/org/mapfish/print/http/HttpRequestFetcher.java +++ b/core/src/main/java/org/mapfish/print/http/HttpRequestFetcher.java @@ -14,20 +14,21 @@ import org.springframework.http.client.ClientHttpRequest; import org.springframework.http.client.ClientHttpResponse; import org.springframework.util.StreamUtils; -import org.springframework.retry.annotation.Retryable; -import org.springframework.retry.annotation.Backoff; import java.io.File; import java.io.FileInputStream; -import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.net.URI; +import java.nio.file.Files; import java.util.concurrent.Callable; import java.util.concurrent.ForkJoinPool; import java.util.concurrent.ForkJoinTask; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import javax.annotation.Nonnull; import javax.annotation.Nullable; /** @@ -39,6 +40,8 @@ public final class HttpRequestFetcher { private static final Logger LOGGER = LoggerFactory.getLogger(HttpRequestFetcher.class); + public static final int MAX_NUMBER_FETCH_RETRY = 3; + public static final int FETCH_RETRY_INTERVAL_MILLIS = 100; private final File temporaryDirectory; @@ -94,12 +97,13 @@ private CachedClientHttpResponse(final ClientHttpResponse originalResponse) thro this.statusText = originalResponse.getStatusText(); this.cachedFile = File.createTempFile("cacheduri", null, HttpRequestFetcher.this.temporaryDirectory); - try (OutputStream os = new FileOutputStream(this.cachedFile)) { + try (OutputStream os = Files.newOutputStream(this.cachedFile.toPath())) { IOUtils.copy(originalResponse.getBody(), os); } } @Override + @Nonnull public InputStream getBody() throws IOException { if (this.body == null) { this.body = new FileInputStream(this.cachedFile); @@ -108,6 +112,7 @@ public InputStream getBody() throws IOException { } @Override + @Nonnull public HttpHeaders getHeaders() { return this.headers; } @@ -118,6 +123,7 @@ public int getRawStatusCode() { } @Override + @Nonnull public String getStatusText() { return this.statusText; } @@ -154,28 +160,33 @@ public HttpMethod getMethod() { } @Override + @Nonnull public String getMethodValue() { final HttpMethod method = this.originalRequest.getMethod(); return method != null ? method.name() : ""; } @Override + @Nonnull public URI getURI() { return this.originalRequest.getURI(); } @Override + @Nonnull public HttpHeaders getHeaders() { return this.originalRequest.getHeaders(); } @Override + @Nonnull public OutputStream getBody() { //body should be written before creating this object throw new UnsupportedOperationException(); } @Override + @Nonnull public ClientHttpResponse execute() { assert this.future != null; final Timer.Context timerWait = @@ -193,14 +204,24 @@ public ClientHttpResponse execute() { return result; } - @Retryable(value = IOException.class, maxAttemptsExpression = "${httpfetch.retry.maxAttempts}", - backoff = @Backoff(delayExpression = "${httpfetch.retry.backoffDelay}")) - private ClientHttpResponse fetch() throws IOException { + private ClientHttpResponse fetch() throws IOException, InterruptedException { LOGGER.debug("Fetching URI resource {}", this.originalRequest.getURI()); - ClientHttpResponse originalResponse = this.originalRequest.execute(); - context.stopIfCanceled(); - return new CachedClientHttpResponse(originalResponse); + AtomicInteger counter = new AtomicInteger(); + do { + try { + ClientHttpResponse originalResponse = this.originalRequest.execute(); + context.stopIfCanceled(); + return new CachedClientHttpResponse(originalResponse); + } catch (final IOException e) { + if (counter.incrementAndGet() < MAX_NUMBER_FETCH_RETRY) { + TimeUnit.MILLISECONDS.sleep(FETCH_RETRY_INTERVAL_MILLIS); + } else { + throw e; + } + } + } while (true); } + @Override public Void call() throws Exception { return context.mdcContextEx(() -> { @@ -215,11 +236,13 @@ public Void call() throws Exception { LOGGER.error("Request failed {}", this.originalRequest.getURI(), e); this.response = new AbstractClientHttpResponse() { @Override + @Nonnull public HttpHeaders getHeaders() { return new HttpHeaders(); } @Override + @Nonnull public InputStream getBody() { return StreamUtils.emptyInput(); } @@ -230,6 +253,7 @@ public int getRawStatusCode() { } @Override + @Nonnull public String getStatusText() { return e.getMessage(); } diff --git a/core/src/main/resources/mapfish-cli-spring-application-context.xml b/core/src/main/resources/mapfish-cli-spring-application-context.xml index 52ff0eeb93..3d191a5e30 100644 --- a/core/src/main/resources/mapfish-cli-spring-application-context.xml +++ b/core/src/main/resources/mapfish-cli-spring-application-context.xml @@ -2,6 +2,7 @@ <beans xmlns="http://www.springframework.org/schema/beans" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans-3.0.xsd"> + <import resource="mapfish-spring-application-context.xml" /> <bean id="servletContext" class="org.mapfish.print.cli.CliServletContext"/> <bean id="main" class="org.mapfish.print.cli.Main"/> diff --git a/core/src/main/resources/mapfish-spring.properties b/core/src/main/resources/mapfish-spring.properties index aca51a9658..cabf73babb 100644 --- a/core/src/main/resources/mapfish-spring.properties +++ b/core/src/main/resources/mapfish-spring.properties @@ -47,6 +47,4 @@ db.schema=public # The default print-apps location printapps.location=servlet:///print-apps -# Configure the retry mechanism for HTTP requests -httpfetch.retry.maxAttempts=2 -httpfetch.retry.backoffDelay=100 + From c86be23939f425b2c44f1fe41fa2876447fe2d8a Mon Sep 17 00:00:00 2001 From: sebr72 <sebastien_riollet@hotmail.com> Date: Mon, 8 May 2023 08:38:12 +0200 Subject: [PATCH 2/4] HttpRequestFetcher is using properties for its retry functionality --- .../print/http/HttpRequestFetcher.java | 25 ++++++++++++------- .../processor/map/CreateMapProcessor.java | 11 +++++++- .../main/resources/mapfish-spring.properties | 4 +++ 3 files changed, 30 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/org/mapfish/print/http/HttpRequestFetcher.java b/core/src/main/java/org/mapfish/print/http/HttpRequestFetcher.java index ff47af31ff..658db92654 100644 --- a/core/src/main/java/org/mapfish/print/http/HttpRequestFetcher.java +++ b/core/src/main/java/org/mapfish/print/http/HttpRequestFetcher.java @@ -40,14 +40,13 @@ public final class HttpRequestFetcher { private static final Logger LOGGER = LoggerFactory.getLogger(HttpRequestFetcher.class); - public static final int MAX_NUMBER_FETCH_RETRY = 3; - public static final int FETCH_RETRY_INTERVAL_MILLIS = 100; private final File temporaryDirectory; - private final MetricRegistry registry; private final Processor.ExecutionContext context; private final ForkJoinPool requestForkJoinPool; + private final int maxNumberFetchRetry; + private final int fetchRetryIntervalMillis; /** * Constructor. @@ -60,11 +59,14 @@ public final class HttpRequestFetcher { public HttpRequestFetcher( final File temporaryDirectory, final MetricRegistry registry, final Processor.ExecutionContext context, - final ForkJoinPool requestForkJoinPool) { + final ForkJoinPool requestForkJoinPool, final int maxNumberFetchRetry, + final int fetchRetryIntervalMillis) { this.temporaryDirectory = temporaryDirectory; this.registry = registry; this.context = context; this.requestForkJoinPool = requestForkJoinPool; + this.maxNumberFetchRetry = maxNumberFetchRetry; + this.fetchRetryIntervalMillis = fetchRetryIntervalMillis; } private CachedClientHttpRequest add(final CachedClientHttpRequest request) { @@ -204,18 +206,23 @@ public ClientHttpResponse execute() { return result; } - private ClientHttpResponse fetch() throws IOException, InterruptedException { - LOGGER.debug("Fetching URI resource {}", this.originalRequest.getURI()); + /** + * Retry implemented manually. + * @see <a href="https://github.com/spring-projects/spring-retry/issues/148">Spring issue 148</a> + */ + private ClientHttpResponse fetchWithRetry() throws IOException, InterruptedException { AtomicInteger counter = new AtomicInteger(); do { try { + LOGGER.debug("Fetching URI resource {}", this.originalRequest.getURI()); ClientHttpResponse originalResponse = this.originalRequest.execute(); context.stopIfCanceled(); return new CachedClientHttpResponse(originalResponse); } catch (final IOException e) { - if (counter.incrementAndGet() < MAX_NUMBER_FETCH_RETRY) { - TimeUnit.MILLISECONDS.sleep(FETCH_RETRY_INTERVAL_MILLIS); + if (counter.incrementAndGet() < HttpRequestFetcher.this.maxNumberFetchRetry) { + TimeUnit.MILLISECONDS.sleep(HttpRequestFetcher.this.fetchRetryIntervalMillis); } else { + LOGGER.debug("Fetching failed after {}", this.originalRequest.getURI()); throw e; } } @@ -231,7 +238,7 @@ public Void call() throws Exception { final Timer.Context timerDownload = HttpRequestFetcher.this.registry.timer(baseMetricName).time(); try { - this.response = this.fetch(); + this.response = this.fetchWithRetry(); } catch (IOException e) { LOGGER.error("Request failed {}", this.originalRequest.getURI(), e); this.response = new AbstractClientHttpResponse() { diff --git a/core/src/main/java/org/mapfish/print/processor/map/CreateMapProcessor.java b/core/src/main/java/org/mapfish/print/processor/map/CreateMapProcessor.java index 2cb4944e2e..fc3a6847de 100644 --- a/core/src/main/java/org/mapfish/print/processor/map/CreateMapProcessor.java +++ b/core/src/main/java/org/mapfish/print/processor/map/CreateMapProcessor.java @@ -56,6 +56,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; import org.w3c.dom.Document; import org.w3c.dom.Element; @@ -113,6 +114,12 @@ public final class CreateMapProcessor @Resource(name = "requestForkJoinPool") private ForkJoinPool requestForkJoinPool; + @Value("${httpRequest.maxNumberFetchRetry}") + private int httpRequestMaxNumberFetchRetry; + + @Value("${httpRequest.fetchRetryIntervalMillis}") + private int httpRequestFetchRetryIntervalMillis; + /** * Constructor. */ @@ -389,7 +396,9 @@ private List<URI> createLayerGraphics( final List<URI> graphics = new ArrayList<>(layers.size()); HttpRequestFetcher cache = new HttpRequestFetcher(printDirectory, this.metricRegistry, - context, this.requestForkJoinPool); + context, this.requestForkJoinPool, + this.httpRequestMaxNumberFetchRetry, + this.httpRequestFetchRetryIntervalMillis); //prepare layers for rendering for (final MapLayer layer: layers) { diff --git a/core/src/main/resources/mapfish-spring.properties b/core/src/main/resources/mapfish-spring.properties index cabf73babb..e2e63db902 100644 --- a/core/src/main/resources/mapfish-spring.properties +++ b/core/src/main/resources/mapfish-spring.properties @@ -47,4 +47,8 @@ db.schema=public # The default print-apps location printapps.location=servlet:///print-apps +# Maximum number of times the same request can be executed if the response was not obtained +httpRequest.maxNumberFetchRetry=3 +# Number of milliseconds between 2 executions of the same request +httpRequest.fetchRetryIntervalMillis=100 From 898043b4a3537fba7d0b36a2d0b1b33187ab64bc Mon Sep 17 00:00:00 2001 From: sebr72 <sebastien_riollet@hotmail.com> Date: Mon, 8 May 2023 09:53:16 +0200 Subject: [PATCH 3/4] Fix javadoc code format --- docs/src/main/resources/templates/docker.html | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/docs/src/main/resources/templates/docker.html b/docs/src/main/resources/templates/docker.html index 0a3f97e417..5360fa4fc0 100644 --- a/docs/src/main/resources/templates/docker.html +++ b/docs/src/main/resources/templates/docker.html @@ -124,17 +124,12 @@ <h4 id="multi"> <p>Example:</p> -<p> - <code> - <pre> +<pre><code> docker run --name=mapfish-print-test --publish=8080:8080 --env=TOMCAT_LOG_TYPE=json --env=CATALINA_OPTS="-Ddb.name=mydb -Ddb.host=myserver -Ddb.username=myuser -Ddb.password=mypwd -Ddb.port=5432" mydockerhub/mapfish-print:latest - </pre - > - </code> -</p> +</code></pre> <p> The necessary tables are created automatically (print_accountings, print_job_results, print_job_statuses). From 152aa461d28d16d76ee06ddd0ab825453b0c6185 Mon Sep 17 00:00:00 2001 From: sebr72 <sebastien_riollet@hotmail.com> Date: Mon, 8 May 2023 10:28:36 +0200 Subject: [PATCH 4/4] Group FetchRetry property names in config --- .../org/mapfish/print/processor/map/CreateMapProcessor.java | 4 ++-- core/src/main/resources/mapfish-spring.properties | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/mapfish/print/processor/map/CreateMapProcessor.java b/core/src/main/java/org/mapfish/print/processor/map/CreateMapProcessor.java index fc3a6847de..2b9e43ef2b 100644 --- a/core/src/main/java/org/mapfish/print/processor/map/CreateMapProcessor.java +++ b/core/src/main/java/org/mapfish/print/processor/map/CreateMapProcessor.java @@ -114,10 +114,10 @@ public final class CreateMapProcessor @Resource(name = "requestForkJoinPool") private ForkJoinPool requestForkJoinPool; - @Value("${httpRequest.maxNumberFetchRetry}") + @Value("${httpRequest.fetchRetry.maxNumber}") private int httpRequestMaxNumberFetchRetry; - @Value("${httpRequest.fetchRetryIntervalMillis}") + @Value("${httpRequest.fetchRetry.intervalMillis}") private int httpRequestFetchRetryIntervalMillis; /** diff --git a/core/src/main/resources/mapfish-spring.properties b/core/src/main/resources/mapfish-spring.properties index e2e63db902..7d59290bee 100644 --- a/core/src/main/resources/mapfish-spring.properties +++ b/core/src/main/resources/mapfish-spring.properties @@ -48,7 +48,7 @@ db.schema=public printapps.location=servlet:///print-apps # Maximum number of times the same request can be executed if the response was not obtained -httpRequest.maxNumberFetchRetry=3 +httpRequest.fetchRetry.maxNumber=3 # Number of milliseconds between 2 executions of the same request -httpRequest.fetchRetryIntervalMillis=100 +httpRequest.fetchRetry.intervalMillis=100