From 148ce3f64aa9b2f643112d9461693c906c933910 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20D=C3=BCmont?= Date: Tue, 18 Nov 2025 09:29:36 +0100 Subject: [PATCH 1/4] Set default retry strategy to no-retries --- .../connectivity/DefaultApacheHttpClient5Factory.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5Factory.java b/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5Factory.java index f7660519b..8f702742a 100644 --- a/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5Factory.java +++ b/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5Factory.java @@ -14,6 +14,7 @@ import org.apache.hc.client5.http.classic.HttpClient; import org.apache.hc.client5.http.config.ConnectionConfig; import org.apache.hc.client5.http.config.RequestConfig; +import org.apache.hc.client5.http.impl.DefaultHttpRequestRetryStrategy; import org.apache.hc.client5.http.impl.classic.CloseableHttpClient; import org.apache.hc.client5.http.impl.classic.HttpClientBuilder; import org.apache.hc.client5.http.impl.classic.HttpClients; @@ -26,6 +27,7 @@ import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.HttpRequestInterceptor; import org.apache.hc.core5.http.io.SocketConfig; +import org.apache.hc.core5.util.TimeValue; import org.apache.hc.core5.util.Timeout; import com.sap.cloud.sdk.cloudplatform.connectivity.exception.DestinationAccessException; @@ -92,6 +94,7 @@ private CloseableHttpClient buildHttpClient( .custom() .setConnectionManager(getConnectionManager(destination)) .setDefaultRequestConfig(requestConfig) + .setRetryStrategy(new DefaultHttpRequestRetryStrategy(0, TimeValue.ZERO_MILLISECONDS)) .setProxy(getProxy(destination)); if( requestInterceptor != null ) { From 7b6ff0df6559a32690dd31b6c21c0e88eb1559cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20D=C3=BCmont?= Date: Thu, 20 Nov 2025 12:35:58 +0100 Subject: [PATCH 2/4] Add retry log message --- .../DefaultApacheHttpClient5Factory.java | 44 ++++++++++++++++++- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5Factory.java b/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5Factory.java index 8f702742a..704785a9d 100644 --- a/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5Factory.java +++ b/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5Factory.java @@ -25,9 +25,11 @@ import org.apache.hc.client5.http.ssl.NoopHostnameVerifier; import org.apache.hc.client5.http.ssl.TlsSocketStrategy; import org.apache.hc.core5.http.HttpHost; +import org.apache.hc.core5.http.HttpRequest; import org.apache.hc.core5.http.HttpRequestInterceptor; +import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.io.SocketConfig; -import org.apache.hc.core5.util.TimeValue; +import org.apache.hc.core5.http.protocol.HttpContext; import org.apache.hc.core5.util.Timeout; import com.sap.cloud.sdk.cloudplatform.connectivity.exception.DestinationAccessException; @@ -94,7 +96,7 @@ private CloseableHttpClient buildHttpClient( .custom() .setConnectionManager(getConnectionManager(destination)) .setDefaultRequestConfig(requestConfig) - .setRetryStrategy(new DefaultHttpRequestRetryStrategy(0, TimeValue.ZERO_MILLISECONDS)) + .setRetryStrategy(new LoggingHttpRequestRetryStrategy(destination)) .setProxy(getProxy(destination)); if( requestInterceptor != null ) { @@ -236,4 +238,42 @@ private boolean isValidProxyConfigurationUriInDestination( @Nullable final HttpD } return true; } + + private static class LoggingHttpRequestRetryStrategy extends DefaultHttpRequestRetryStrategy + { + final @Nullable HttpDestinationProperties destination; + + public LoggingHttpRequestRetryStrategy( final @Nullable HttpDestinationProperties destination ) + { + super(); + this.destination = destination; + } + + @Override + public boolean retryRequest( final HttpResponse response, final int execCount, final HttpContext context ) + { + final boolean retry = super.retryRequest(response, execCount, context); + if( retry ) { + final String msg = "Retrying request for destination {} due to response {}. Retry attempt #{}."; + log.debug(msg, destination, response.getCode(), execCount); + } + return retry; + } + + @Override + public boolean retryRequest( + final HttpRequest req, + final IOException exception, + final int execCount, + final HttpContext context ) + { + final boolean retry = super.retryRequest(req, exception, execCount, context); + if( retry ) { + final String msg = + "Retrying {} request for destination {} to {} due to exception \"{}\". Retry attempt #{}."; + log.debug(msg, req.getMethod(), destination, req.getRequestUri(), exception.getMessage(), execCount); + } + return retry; + } + } } From a337dcec79ff29293430a92a7326d763b80ff081 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20D=C3=BCmont?= Date: Fri, 24 Apr 2026 13:08:18 +0200 Subject: [PATCH 3/4] Improve logging; Add test --- .../DefaultApacheHttpClient5Factory.java | 27 +++- .../LoggingHttpRequestRetryStrategyTest.java | 143 ++++++++++++++++++ 2 files changed, 163 insertions(+), 7 deletions(-) create mode 100644 cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/LoggingHttpRequestRetryStrategyTest.java diff --git a/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5Factory.java b/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5Factory.java index 704785a9d..df3547db3 100644 --- a/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5Factory.java +++ b/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5Factory.java @@ -30,6 +30,7 @@ import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.io.SocketConfig; import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.hc.core5.util.TimeValue; import org.apache.hc.core5.util.Timeout; import com.sap.cloud.sdk.cloudplatform.connectivity.exception.DestinationAccessException; @@ -241,12 +242,15 @@ private boolean isValidProxyConfigurationUriInDestination( @Nullable final HttpD private static class LoggingHttpRequestRetryStrategy extends DefaultHttpRequestRetryStrategy { - final @Nullable HttpDestinationProperties destination; + private static final int MAX_RETRIES = 1; // default + private static final Duration RETRY_INTERVAL = Duration.ofSeconds(1L); // default + + private final @Nonnull String destinationRef; public LoggingHttpRequestRetryStrategy( final @Nullable HttpDestinationProperties destination ) { - super(); - this.destination = destination; + super(MAX_RETRIES, TimeValue.of(RETRY_INTERVAL)); + this.destinationRef = destination == null ? "" : " for destination " + destination; } @Override @@ -254,8 +258,8 @@ public boolean retryRequest( final HttpResponse response, final int execCount, f { final boolean retry = super.retryRequest(response, execCount, context); if( retry ) { - final String msg = "Retrying request for destination {} due to response {}. Retry attempt #{}."; - log.debug(msg, destination, response.getCode(), execCount); + final String msg = "Retrying request{} due to response {}. Retry attempt {}/{} after {}s."; + log.warn(msg, destinationRef, response.getCode(), execCount, MAX_RETRIES, RETRY_INTERVAL.getSeconds()); } return retry; } @@ -270,8 +274,17 @@ public boolean retryRequest( final boolean retry = super.retryRequest(req, exception, execCount, context); if( retry ) { final String msg = - "Retrying {} request for destination {} to {} due to exception \"{}\". Retry attempt #{}."; - log.debug(msg, req.getMethod(), destination, req.getRequestUri(), exception.getMessage(), execCount); + "Retrying {} request{} to {} due to exception \"{}\". Retry attempt {}/{} after {}s."; + log + .warn( + msg, + req.getMethod(), + destinationRef, + req.getRequestUri(), + exception.getMessage(), + execCount, + MAX_RETRIES, + RETRY_INTERVAL.getSeconds()); } return retry; } diff --git a/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/LoggingHttpRequestRetryStrategyTest.java b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/LoggingHttpRequestRetryStrategyTest.java new file mode 100644 index 000000000..3c72ae0e0 --- /dev/null +++ b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/LoggingHttpRequestRetryStrategyTest.java @@ -0,0 +1,143 @@ +package com.sap.cloud.sdk.cloudplatform.connectivity; + +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.get; +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static com.github.tomakehurst.wiremock.stubbing.Scenario.STARTED; +import static org.assertj.core.api.Assertions.assertThat; + +import java.io.IOException; + +import org.apache.hc.client5.http.classic.methods.HttpGet; +import org.apache.hc.client5.http.impl.classic.CloseableHttpClient; +import org.apache.hc.core5.http.HttpResponse; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import com.github.tomakehurst.wiremock.junit5.WireMockExtension; + +class LoggingHttpRequestRetryStrategyTest +{ + @RegisterExtension + static final WireMockExtension WIRE_MOCK_SERVER = + WireMockExtension.newInstance().options(wireMockConfig().dynamicPort()).build(); + + @Test + void testHttpClientRetriesOn503() + throws IOException + { + // Use WireMock Scenarios to simulate: first request returns 503, second returns 200 + WIRE_MOCK_SERVER + .stubFor( + get(urlEqualTo("/retry-test")) + .inScenario("Retry 503") + .whenScenarioStateIs(STARTED) + .willReturn(aResponse().withStatus(503)) + .willSetStateTo("Recovered")); + + WIRE_MOCK_SERVER + .stubFor( + get(urlEqualTo("/retry-test")) + .inScenario("Retry 503") + .whenScenarioStateIs("Recovered") + .willReturn(aResponse().withStatus(200).withBody("OK"))); + + final ApacheHttpClient5Factory factory = new ApacheHttpClient5FactoryBuilder().build(); + + try( CloseableHttpClient client = (CloseableHttpClient) factory.createHttpClient() ) { + final HttpGet request = new HttpGet(WIRE_MOCK_SERVER.url("/retry-test")); + final int statusCode = client.execute(request, HttpResponse::getCode); + + assertThat(statusCode).isEqualTo(200); + WIRE_MOCK_SERVER.verify(2, getRequestedFor(urlEqualTo("/retry-test"))); + } + } + + @Test + void testHttpClientRetriesOn429() + throws IOException + { + // Use WireMock Scenarios to simulate: first request returns 429, second returns 200 + WIRE_MOCK_SERVER + .stubFor( + get(urlEqualTo("/rate-limit-test")) + .inScenario("Retry 429") + .whenScenarioStateIs(STARTED) + .willReturn(aResponse().withStatus(429)) + .willSetStateTo("Recovered")); + + WIRE_MOCK_SERVER + .stubFor( + get(urlEqualTo("/rate-limit-test")) + .inScenario("Retry 429") + .whenScenarioStateIs("Recovered") + .willReturn(aResponse().withStatus(200).withBody("OK"))); + + final ApacheHttpClient5Factory factory = new ApacheHttpClient5FactoryBuilder().build(); + + try( CloseableHttpClient client = (CloseableHttpClient) factory.createHttpClient() ) { + final HttpGet request = new HttpGet(WIRE_MOCK_SERVER.url("/rate-limit-test")); + final int statusCode = client.execute(request, HttpResponse::getCode); + + assertThat(statusCode).isEqualTo(200); + WIRE_MOCK_SERVER.verify(2, getRequestedFor(urlEqualTo("/rate-limit-test"))); + } + } + + @Test + void testHttpClientDoesNotRetryOn500() + throws IOException + { + // 500 Internal Server Error should not trigger retry + WIRE_MOCK_SERVER.stubFor(get(urlEqualTo("/no-retry-test")).willReturn(aResponse().withStatus(500))); + + final ApacheHttpClient5Factory factory = new ApacheHttpClient5FactoryBuilder().build(); + + try( CloseableHttpClient client = (CloseableHttpClient) factory.createHttpClient() ) { + final HttpGet request = new HttpGet(WIRE_MOCK_SERVER.url("/no-retry-test")); + final int statusCode = client.execute(request, HttpResponse::getCode); + + assertThat(statusCode).isEqualTo(500); + WIRE_MOCK_SERVER.verify(1, getRequestedFor(urlEqualTo("/no-retry-test"))); + } + } + + @Test + void testHttpClientDoesNotRetryOn400() + throws IOException + { + // 400 Bad Request should not trigger retry + WIRE_MOCK_SERVER.stubFor(get(urlEqualTo("/bad-request-test")).willReturn(aResponse().withStatus(400))); + + final ApacheHttpClient5Factory factory = new ApacheHttpClient5FactoryBuilder().build(); + + try( CloseableHttpClient client = (CloseableHttpClient) factory.createHttpClient() ) { + final HttpGet request = new HttpGet(WIRE_MOCK_SERVER.url("/bad-request-test")); + final int statusCode = client.execute(request, HttpResponse::getCode); + + assertThat(statusCode).isEqualTo(400); + WIRE_MOCK_SERVER.verify(1, getRequestedFor(urlEqualTo("/bad-request-test"))); + } + } + + @Test + void testHttpClientStopsRetryingAfterMaxAttempts() + throws IOException + { + // Server always returns 503 - should stop after max retries (1 retry = 2 total requests) + WIRE_MOCK_SERVER.stubFor(get(urlEqualTo("/always-503")).willReturn(aResponse().withStatus(503))); + + final ApacheHttpClient5Factory factory = new ApacheHttpClient5FactoryBuilder().build(); + + try( CloseableHttpClient client = (CloseableHttpClient) factory.createHttpClient() ) { + final HttpGet request = new HttpGet(WIRE_MOCK_SERVER.url("/always-503")); + final int statusCode = client.execute(request, HttpResponse::getCode); + + // DefaultHttpRequestRetryStrategy allows 1 retry, so 2 total requests + assertThat(statusCode).isEqualTo(503); + WIRE_MOCK_SERVER.verify(2, getRequestedFor(urlEqualTo("/always-503"))); + } + } +} From f4996ec107253f1c8bdf6f7a0752abe5ab1d2db5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20D=C3=BCmont?= Date: Fri, 24 Apr 2026 13:11:43 +0200 Subject: [PATCH 4/4] Update release_notes.md --- release_notes.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/release_notes.md b/release_notes.md index 3112514a9..6e2985406 100644 --- a/release_notes.md +++ b/release_notes.md @@ -16,7 +16,7 @@ ### 📈 Improvements -- +- Enable Retry behavior for HttpClient5 instances, to mirror legacy behvior for HttpClient4: `1` retry with `1s` delay for response codes `429` (Too Many Requests) and `503` (Service Unavailable). ### 🐛 Fixed Issues