This is an automated email from the ASF dual-hosted git repository.
huaxingao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg.git
The following commit(s) were added to refs/heads/main by this push:
new 7d6d369157 Core: Bound Idempotency-Key retries to the advertised key
lifetime (#18070)
7d6d369157 is described below
commit 7d6d369157f61be85add25aaf2dfcc78f456ccbc
Author: Sushant Raikar <[email protected]>
AuthorDate: Mon Sep 28 23:15:55 2026 -0700
Core: Bound Idempotency-Key retries to the advertised key lifetime (#18070)
Auto-retrying a keyed mutation could previously fire after the server's
advertised Idempotency-Key lifetime elapsed, at which point the server no
longer dedupes the key and could apply the mutation twice. This records when a
request is first sent (in HTTPClient.execute) and, for requests carrying an
Idempotency-Key, declines a retry once the elapsed time since the first attempt
plus the next retry interval would reach the advertised lifetime. The lifetime
from GET /v1/config is plumbe [...]
Per Yufei's review, the check compares against the upper bound of the next
retry interval (Retry-After verbatim, or exponential backoff plus its maximum
jitter) rather than a freshly sampled value, so a retry the check permits
cannot then sleep longer than checked and overshoot the window.
---
.../rest/ExponentialHttpRequestRetryStrategy.java | 130 +++++++++++----
.../java/org/apache/iceberg/rest/HTTPClient.java | 32 +++-
.../apache/iceberg/rest/RESTSessionCatalog.java | 7 +-
.../TestExponentialHttpRequestRetryStrategy.java | 182 +++++++++++++++++++++
.../org/apache/iceberg/rest/TestHTTPClient.java | 20 +++
5 files changed, 333 insertions(+), 38 deletions(-)
diff --git
a/core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java
b/core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java
index 6ae57e6d2c..7356575ade 100644
---
a/core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java
+++
b/core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java
@@ -23,6 +23,7 @@ import java.io.InterruptedIOException;
import java.net.ConnectException;
import java.net.NoRouteToHostException;
import java.net.UnknownHostException;
+import java.time.Duration;
import java.time.Instant;
import java.util.Set;
import java.util.concurrent.ThreadLocalRandom;
@@ -42,6 +43,8 @@ import org.apache.hc.core5.http.protocol.HttpCoreContext;
import org.apache.hc.core5.util.TimeValue;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Defines an exponential HTTP request retry strategy and provides the same
characteristics as the
@@ -82,15 +85,26 @@ import
org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
* {@link #getRetryInterval(HttpResponse, int, HttpContext)} to achieve
exponential backoff.
*/
class ExponentialHttpRequestRetryStrategy implements HttpRequestRetryStrategy {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(ExponentialHttpRequestRetryStrategy.class);
+
+ static final String FIRST_ATTEMPT_EPOCH = "first-attempt-epoch";
+
private final int maxRetries;
+ private final Duration keyLifetime;
private final Set<Class<? extends IOException>> nonRetriableExceptions;
private final Set<Integer> retriableCodes;
private final Set<Integer> idempotentRetriableCodes;
ExponentialHttpRequestRetryStrategy(int maximumRetries) {
+ this(maximumRetries, null);
+ }
+
+ ExponentialHttpRequestRetryStrategy(int maximumRetries, Duration
keyLifetime) {
Preconditions.checkArgument(
maximumRetries > 0, "Cannot set retries to %s, the value must be
positive", maximumRetries);
this.maxRetries = maximumRetries;
+ this.keyLifetime = keyLifetime;
this.retriableCodes = ImmutableSet.of(HttpStatus.SC_TOO_MANY_REQUESTS);
this.idempotentRetriableCodes =
ImmutableSet.of(
@@ -133,10 +147,11 @@ class ExponentialHttpRequestRetryStrategy implements
HttpRequestRetryStrategy {
return false;
}
- // Retry if the request is idempotent, or carries an Idempotency-Key
(server guarantees safe
- // retry)
- return Method.isIdempotent(request.getMethod())
- || request.containsHeader(RESTUtil.IDEMPOTENCY_KEY_HEADER);
+ if (!isSafeToRetry(request)) {
+ return false;
+ }
+
+ return !wouldExceedKeyLifetime(request, null, execCount, context);
}
@Override
@@ -154,51 +169,94 @@ class ExponentialHttpRequestRetryStrategy implements
HttpRequestRetryStrategy {
// - It's in a predefined list of retriable codes.
// - The request is idempotent, and the response code indicates a retry
is safe.
// - The response code is '503 Service Unavailable' and includes a
'Retry-After' header.
- return execCount <= maxRetries
- && (retriableCodes.contains(response.getCode())
- || shouldRetryIdempotent(request, response.getCode())
- || is503Retryable);
+ boolean shouldRetry =
+ execCount <= maxRetries
+ && (retriableCodes.contains(response.getCode())
+ || (isSafeToRetry(request) &&
idempotentRetriableCodes.contains(response.getCode()))
+ || is503Retryable);
+
+ if (!shouldRetry) {
+ return false;
+ }
+
+ return !wouldExceedKeyLifetime(request, response, execCount, context);
}
@Override
public TimeValue getRetryInterval(HttpResponse response, int execCount,
HttpContext context) {
- // a server may send a 429 / 503 with a Retry-After header
- // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After
+ TimeValue retryAfter = retryAfter(response);
+ if (TimeValue.isPositive(retryAfter)) {
+ return retryAfter;
+ }
+ long delayMillis = nextBackoffMillis(execCount);
+ int jitter = ThreadLocalRandom.current().nextInt(Math.max(1, (int)
(delayMillis * 0.1)));
+ return TimeValue.ofMilliseconds(delayMillis + jitter);
+ }
+
+ // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After
(seconds or HTTP-date)
+ private static TimeValue retryAfter(HttpResponse response) {
Header header = response.getFirstHeader(HttpHeaders.RETRY_AFTER);
- TimeValue retryAfter = null;
- if (header != null) {
- String value = header.getValue();
- try {
- retryAfter = TimeValue.ofSeconds(Long.parseLong(value));
- } catch (NumberFormatException ignore) {
- Instant retryAfterDate = DateUtils.parseStandardDate(value);
- if (retryAfterDate != null) {
- retryAfter =
- TimeValue.ofMilliseconds(retryAfterDate.toEpochMilli() -
System.currentTimeMillis());
- }
- }
+ if (header == null) {
+ return null;
+ }
+ String value = header.getValue();
+ try {
+ return TimeValue.ofSeconds(Long.parseLong(value));
+ } catch (NumberFormatException ignore) {
+ Instant date = DateUtils.parseStandardDate(value);
+ return date == null
+ ? null
+ : TimeValue.ofMilliseconds(date.toEpochMilli() -
System.currentTimeMillis());
+ }
+ }
- if (TimeValue.isPositive(retryAfter)) {
- return retryAfter;
- }
+ // Upper bound of getRetryInterval: Retry-After verbatim (no jitter), else
backoff + max 10%
+ // jitter.
+ // The lifetime check uses this so a permitted retry can't then sleep longer
than checked.
+ private static long maxRetryIntervalMillis(HttpResponse response, int
execCount) {
+ TimeValue retryAfter = retryAfter(response);
+ if (TimeValue.isPositive(retryAfter)) {
+ return retryAfter.toMilliseconds();
}
+ long delayMillis = nextBackoffMillis(execCount);
+ return delayMillis + (long) (delayMillis * 0.1);
+ }
- int delayMillis = 1000 * (int) Math.min(Math.pow(2.0, (long) execCount -
1.0), 64.0);
- int jitter = ThreadLocalRandom.current().nextInt(Math.max(1, (int)
(delayMillis * 0.1)));
+ private static boolean isSafeToRetry(HttpRequest request) {
+ return request != null
+ && (Method.isIdempotent(request.getMethod())
+ || request.containsHeader(RESTUtil.IDEMPOTENCY_KEY_HEADER));
+ }
- return TimeValue.ofMilliseconds(delayMillis + jitter);
+ private static long nextBackoffMillis(int execCount) {
+ return 1000L * (long) Math.min(Math.pow(2.0, (long) execCount - 1.0),
64.0);
}
- private boolean shouldRetryIdempotent(HttpRequest request, int responseCode)
{
- if (request == null) {
+ private boolean wouldExceedKeyLifetime(
+ HttpRequest request, HttpResponse response, int execCount, HttpContext
context) {
+ if (keyLifetime == null
+ || context == null
+ || request == null
+ || !request.containsHeader(RESTUtil.IDEMPOTENCY_KEY_HEADER)) {
+ return false;
+ }
+
+ Object attr = context.getAttribute(FIRST_ATTEMPT_EPOCH);
+ if (!(attr instanceof Long)) {
+ LOG.warn(
+ "Idempotency-Key lifetime not enforced: first-attempt time not
recorded for {}",
+ request.getMethod());
return false;
}
- // A request is retry-safe if its HTTP method is idempotent or it carries
an Idempotency-Key
- // header (which lets the server replay a finalized result on retry).
- boolean retrySafe =
- Method.isIdempotent(request.getMethod())
- || request.containsHeader(RESTUtil.IDEMPOTENCY_KEY_HEADER);
- return retrySafe && idempotentRetriableCodes.contains(responseCode);
+ long firstAttemptMillis = (Long) attr;
+ long now = System.currentTimeMillis();
+ long nextIntervalMillis =
+ response != null
+ ? maxRetryIntervalMillis(response, execCount)
+ : nextBackoffMillis(execCount);
+ boolean nextAttemptExceedsLifetime =
+ (now - firstAttemptMillis) + nextIntervalMillis >=
keyLifetime.toMillis();
+ return nextAttemptExceedsLifetime;
}
}
diff --git a/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java
b/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java
index e2f69c29b0..100232eb83 100644
--- a/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java
+++ b/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java
@@ -24,6 +24,8 @@ import java.io.IOException;
import java.io.UncheckedIOException;
import java.net.URI;
import java.nio.charset.StandardCharsets;
+import java.time.Duration;
+import java.time.format.DateTimeParseException;
import java.util.Map;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.TimeUnit;
@@ -81,6 +83,7 @@ public class HTTPClient extends BaseHTTPClient {
static final String CLIENT_GIT_COMMIT_SHORT_HEADER =
"X-Client-Git-Commit-Short";
private static final String REST_MAX_RETRIES = "rest.client.max-retries";
+ static final String REST_IDEMPOTENCY_KEY_LIFETIME =
"rest.client.idempotency-key-lifetime";
static final String REST_MAX_CONNECTIONS = "rest.client.max-connections";
static final int REST_MAX_CONNECTIONS_DEFAULT = 100;
static final String REST_MAX_CONNECTIONS_PER_ROUTE =
"rest.client.connections-per-route";
@@ -125,7 +128,9 @@ public class HTTPClient extends BaseHTTPClient {
clientBuilder.setConnectionManager(connectionManager);
int maxRetries = PropertyUtil.propertyAsInt(properties, REST_MAX_RETRIES,
5);
- clientBuilder.setRetryStrategy(new
ExponentialHttpRequestRetryStrategy(maxRetries));
+ Duration keyLifetime =
parseKeyLifetime(properties.get(REST_IDEMPOTENCY_KEY_LIFETIME));
+ clientBuilder.setRetryStrategy(
+ new ExponentialHttpRequestRetryStrategy(maxRetries, keyLifetime));
String userAgent = PropertyUtil.propertyAsString(properties,
REST_USER_AGENT, null);
if (userAgent != null) {
@@ -164,6 +169,28 @@ public class HTTPClient extends BaseHTTPClient {
return new HTTPClient(this, session);
}
+ @VisibleForTesting
+ static Duration parseKeyLifetime(String lifetime) {
+ if (lifetime == null) {
+ return null;
+ }
+ try {
+ Duration parsed = Duration.parse(lifetime);
+ if (parsed.isZero() || parsed.isNegative()) {
+ // Non-positive window: never retry keyed requests (null would instead
disable the bound).
+ return Duration.ZERO;
+ }
+ if (parsed.compareTo(Duration.ofMillis(Long.MAX_VALUE)) > 0) {
+ LOG.warn("Ignoring out-of-range idempotency-key lifetime: {}",
lifetime);
+ return null;
+ }
+ return parsed;
+ } catch (DateTimeParseException | ArithmeticException e) {
+ LOG.warn("Ignoring malformed idempotency-key lifetime: {}", lifetime, e);
+ return null;
+ }
+ }
+
private static String extractResponseBodyAsString(ClassicHttpResponse
response) {
try {
if (response.getEntity() == null) {
@@ -317,6 +344,9 @@ public class HTTPClient extends BaseHTTPClient {
}
HttpContext context = HttpClientContext.create();
+ // Record first-send time so the retry strategy bounds keyed retries to
the key lifetime.
+ context.setAttribute(
+ ExponentialHttpRequestRetryStrategy.FIRST_ATTEMPT_EPOCH,
System.currentTimeMillis());
try {
return httpClient.execute(
request,
diff --git a/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
b/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
index 0b30ef9d88..663d28f6b8 100644
--- a/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
+++ b/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
@@ -215,13 +215,18 @@ public class RESTSessionCatalog extends
BaseViewSessionCatalog
}
// build the final configuration and set up the catalog's auth
- Map<String, String> mergedProps = config.merge(props);
+ ImmutableMap.Builder<String, String> mergedPropsBuilder =
+ ImmutableMap.<String, String>builder().putAll(config.merge(props));
// Enable Idempotency-Key header for mutation endpoints if the server
advertises support
if (config.idempotencyKeyLifetime() != null) {
this.mutationHeaders = RESTUtil::idempotencyHeaders;
+ mergedPropsBuilder.put(
+ HTTPClient.REST_IDEMPOTENCY_KEY_LIFETIME,
config.idempotencyKeyLifetime());
}
+ Map<String, String> mergedProps = mergedPropsBuilder.buildKeepingLast();
+
if (config.endpoints().isEmpty()) {
this.endpoints =
PropertyUtil.propertyAsBoolean(
diff --git
a/core/src/test/java/org/apache/iceberg/rest/TestExponentialHttpRequestRetryStrategy.java
b/core/src/test/java/org/apache/iceberg/rest/TestExponentialHttpRequestRetryStrategy.java
index 7dcacc3a12..b4e4cd5676 100644
---
a/core/src/test/java/org/apache/iceberg/rest/TestExponentialHttpRequestRetryStrategy.java
+++
b/core/src/test/java/org/apache/iceberg/rest/TestExponentialHttpRequestRetryStrategy.java
@@ -27,6 +27,7 @@ import java.net.ConnectException;
import java.net.NoRouteToHostException;
import java.net.SocketTimeoutException;
import java.net.UnknownHostException;
+import java.time.Duration;
import java.time.Instant;
import java.time.temporal.ChronoUnit;
import javax.net.ssl.SSLException;
@@ -249,4 +250,185 @@ public class TestExponentialHttpRequestRetryStrategy {
context.setRequest(new BasicHttpRequest("POST", "/"));
assertThat(retryStrategy.retryRequest(response, 3, context)).isFalse();
}
+
+ @Test
+ public void testKeyedRetryAbandonedWhenLifetimeExceeded() {
+ HttpRequestRetryStrategy strategy =
+ new ExponentialHttpRequestRetryStrategy(5, Duration.ofSeconds(1));
+ BasicHttpResponse response =
+ new BasicHttpResponse(HttpStatus.SC_INTERNAL_SERVER_ERROR, "error");
+ response.addHeader(new BasicHeader(HttpHeaders.RETRY_AFTER, "3600"));
+ HttpClientContext context = HttpClientContext.create();
+ BasicHttpRequest request = new BasicHttpRequest("POST", "/");
+ request.addHeader(RESTUtil.IDEMPOTENCY_KEY_HEADER, "key-abandon");
+ context.setRequest(request);
+ context.setAttribute(
+ ExponentialHttpRequestRetryStrategy.FIRST_ATTEMPT_EPOCH,
System.currentTimeMillis());
+ assertThat(strategy.retryRequest(response, 1, context)).isFalse();
+ }
+
+ @Test
+ public void testKeyedRetryContinuesWithinLifetime() {
+ HttpRequestRetryStrategy strategy =
+ new ExponentialHttpRequestRetryStrategy(5, Duration.ofHours(1));
+ BasicHttpResponse response =
+ new BasicHttpResponse(HttpStatus.SC_INTERNAL_SERVER_ERROR, "error");
+ response.addHeader(new BasicHeader(HttpHeaders.RETRY_AFTER, "1"));
+ HttpClientContext context = HttpClientContext.create();
+ BasicHttpRequest request = new BasicHttpRequest("POST", "/");
+ request.addHeader(RESTUtil.IDEMPOTENCY_KEY_HEADER, "key-continue");
+ context.setRequest(request);
+ context.setAttribute(
+ ExponentialHttpRequestRetryStrategy.FIRST_ATTEMPT_EPOCH,
System.currentTimeMillis());
+ assertThat(strategy.retryRequest(response, 1, context)).isTrue();
+ }
+
+ @Test
+ public void testKeyedNetworkExceptionRetryAbandonedWhenLifetimeExceeded() {
+ HttpRequestRetryStrategy strategy =
+ new ExponentialHttpRequestRetryStrategy(5, Duration.ofMillis(1));
+ HttpClientContext context = HttpClientContext.create();
+ BasicHttpRequest request = new BasicHttpRequest("POST", "/");
+ request.addHeader(RESTUtil.IDEMPOTENCY_KEY_HEADER, "key-net-abandon");
+ context.setRequest(request);
+ context.setAttribute(
+ ExponentialHttpRequestRetryStrategy.FIRST_ATTEMPT_EPOCH,
System.currentTimeMillis());
+ assertThat(strategy.retryRequest(request, new IOException("connection
reset"), 1, context))
+ .isFalse();
+ }
+
+ @Test
+ public void testKeyedNetworkExceptionRetryHonoredWithoutLifetime() {
+ HttpRequestRetryStrategy strategy = new
ExponentialHttpRequestRetryStrategy(5);
+ HttpClientContext context = HttpClientContext.create();
+ BasicHttpRequest request = new BasicHttpRequest("POST", "/");
+ request.addHeader(RESTUtil.IDEMPOTENCY_KEY_HEADER, "key-net-ok");
+ context.setRequest(request);
+ assertThat(strategy.retryRequest(request, new IOException("connection
reset"), 1, context))
+ .isTrue();
+ }
+
+ @Test
+ public void testIdempotentGetNotConstrainedByKeyLifetime() {
+ HttpRequestRetryStrategy strategy =
+ new ExponentialHttpRequestRetryStrategy(5, Duration.ofMillis(1));
+ BasicHttpResponse response =
+ new BasicHttpResponse(HttpStatus.SC_INTERNAL_SERVER_ERROR, "error");
+ response.addHeader(new BasicHeader(HttpHeaders.RETRY_AFTER, "3600"));
+ HttpClientContext context = HttpClientContext.create();
+ context.setRequest(new BasicHttpRequest("GET", "/"));
+ assertThat(strategy.retryRequest(response, 1, context)).isTrue();
+ }
+
+ @Test
+ public void testKeyedRetryHonorsRecordedFirstAttemptTime() {
+ HttpRequestRetryStrategy strategy =
+ new ExponentialHttpRequestRetryStrategy(5, Duration.ofMinutes(30));
+ BasicHttpResponse response =
+ new BasicHttpResponse(HttpStatus.SC_INTERNAL_SERVER_ERROR, "error");
+ response.addHeader(new BasicHeader(HttpHeaders.RETRY_AFTER, "1"));
+ HttpClientContext context = HttpClientContext.create();
+ BasicHttpRequest request = new BasicHttpRequest("POST", "/");
+ request.addHeader(RESTUtil.IDEMPOTENCY_KEY_HEADER, "key-recorded");
+ context.setRequest(request);
+ context.setAttribute(
+ ExponentialHttpRequestRetryStrategy.FIRST_ATTEMPT_EPOCH,
+ System.currentTimeMillis() - Duration.ofMinutes(31).toMillis());
+ assertThat(strategy.retryRequest(response, 1, context)).isFalse();
+ }
+
+ @Test
+ public void testKeyedRetryAbandonedOnceCumulativeElapsedExceedsLifetime()
+ throws InterruptedException {
+ // 2s lifetime, 1s Retry-After: 1st retry allowed, 2nd abandoned once
cumulative elapsed > 2s.
+ HttpRequestRetryStrategy strategy =
+ new ExponentialHttpRequestRetryStrategy(5, Duration.ofSeconds(2));
+ BasicHttpResponse response =
+ new BasicHttpResponse(HttpStatus.SC_INTERNAL_SERVER_ERROR, "error");
+ response.addHeader(new BasicHeader(HttpHeaders.RETRY_AFTER, "1"));
+ HttpClientContext context = HttpClientContext.create();
+ BasicHttpRequest request = new BasicHttpRequest("POST", "/");
+ request.addHeader(RESTUtil.IDEMPOTENCY_KEY_HEADER, "key-cumulative");
+ context.setRequest(request);
+ context.setAttribute(
+ ExponentialHttpRequestRetryStrategy.FIRST_ATTEMPT_EPOCH,
System.currentTimeMillis());
+
+ assertThat(strategy.retryRequest(response, 1, context)).isTrue();
+ Thread.sleep(1100);
+ assertThat(strategy.retryRequest(response, 2, context)).isFalse();
+ }
+
+ @Test
+ public void testKeyedRetryProceedsWhenFirstAttemptTimeNotRecorded() {
+ HttpRequestRetryStrategy strategy =
+ new ExponentialHttpRequestRetryStrategy(5, Duration.ofSeconds(1));
+ BasicHttpResponse response =
+ new BasicHttpResponse(HttpStatus.SC_INTERNAL_SERVER_ERROR, "error");
+ response.addHeader(new BasicHeader(HttpHeaders.RETRY_AFTER, "3600"));
+ HttpClientContext context = HttpClientContext.create();
+ BasicHttpRequest request = new BasicHttpRequest("POST", "/");
+ request.addHeader(RESTUtil.IDEMPOTENCY_KEY_HEADER, "key-unrecorded");
+ context.setRequest(request);
+ assertThat(strategy.retryRequest(response, 1, context)).isTrue();
+ }
+
+ @Test
+ public void testKeyedRetryAbandonedWhenExponentialIntervalExceedsLifetime() {
+ // No Retry-After -> interval falls back to exponential (>=1s), exceeding
the 1ms lifetime.
+ HttpRequestRetryStrategy strategy =
+ new ExponentialHttpRequestRetryStrategy(5, Duration.ofMillis(1));
+ BasicHttpResponse response =
+ new BasicHttpResponse(HttpStatus.SC_INTERNAL_SERVER_ERROR, "error");
+ HttpClientContext context = HttpClientContext.create();
+ BasicHttpRequest request = new BasicHttpRequest("POST", "/");
+ request.addHeader(RESTUtil.IDEMPOTENCY_KEY_HEADER, "key-exp");
+ context.setRequest(request);
+ context.setAttribute(
+ ExponentialHttpRequestRetryStrategy.FIRST_ATTEMPT_EPOCH,
System.currentTimeMillis());
+ assertThat(strategy.retryRequest(response, 1, context)).isFalse();
+ }
+
+ @Test
+ public void testKeyedRetryAlwaysAbandonedWithZeroLifetime() {
+ HttpRequestRetryStrategy strategy = new
ExponentialHttpRequestRetryStrategy(5, Duration.ZERO);
+ BasicHttpResponse response =
+ new BasicHttpResponse(HttpStatus.SC_INTERNAL_SERVER_ERROR, "error");
+ response.addHeader(new BasicHeader(HttpHeaders.RETRY_AFTER, "1"));
+ HttpClientContext context = HttpClientContext.create();
+ BasicHttpRequest request = new BasicHttpRequest("POST", "/");
+ request.addHeader(RESTUtil.IDEMPOTENCY_KEY_HEADER, "key-zero");
+ context.setRequest(request);
+ context.setAttribute(
+ ExponentialHttpRequestRetryStrategy.FIRST_ATTEMPT_EPOCH,
System.currentTimeMillis());
+ assertThat(strategy.retryRequest(response, 1, context)).isFalse();
+ }
+
+ @Test
+ public void testIdempotentGetStillRetriesWithZeroLifetime() {
+ HttpRequestRetryStrategy strategy = new
ExponentialHttpRequestRetryStrategy(5, Duration.ZERO);
+ BasicHttpResponse response =
+ new BasicHttpResponse(HttpStatus.SC_INTERNAL_SERVER_ERROR, "error");
+ response.addHeader(new BasicHeader(HttpHeaders.RETRY_AFTER, "1"));
+ HttpClientContext context = HttpClientContext.create();
+ context.setRequest(new BasicHttpRequest("GET", "/"));
+ assertThat(strategy.retryRequest(response, 1, context)).isTrue();
+ }
+
+ @Test
+ public void testKeyedRetryAbandonedWhenMaxJitterWouldExceedLifetime() {
+ // Lifetime sits between base backoff (1s) and base + max jitter (1.1s);
the check uses the
+ // upper
+ // bound, so the retry is abandoned even though a low-jitter sleep might
have fit.
+ HttpRequestRetryStrategy strategy =
+ new ExponentialHttpRequestRetryStrategy(5, Duration.ofMillis(1050));
+ BasicHttpResponse response =
+ new BasicHttpResponse(HttpStatus.SC_INTERNAL_SERVER_ERROR, "error");
+ HttpClientContext context = HttpClientContext.create();
+ BasicHttpRequest request = new BasicHttpRequest("POST", "/");
+ request.addHeader(RESTUtil.IDEMPOTENCY_KEY_HEADER, "key-maxjitter");
+ context.setRequest(request);
+ context.setAttribute(
+ ExponentialHttpRequestRetryStrategy.FIRST_ATTEMPT_EPOCH,
System.currentTimeMillis());
+ assertThat(strategy.retryRequest(response, 1, context)).isFalse();
+ }
}
diff --git a/core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java
b/core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java
index 14d898e432..2c058ffc48 100644
--- a/core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java
+++ b/core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java
@@ -38,6 +38,7 @@ import java.net.SocketTimeoutException;
import java.nio.file.Path;
import java.security.KeyStore;
import java.security.cert.CertificateException;
+import java.time.Duration;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
@@ -71,6 +72,7 @@ import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.EnumSource;
+import org.junit.jupiter.params.provider.NullSource;
import org.junit.jupiter.params.provider.ValueSource;
import org.mockserver.configuration.Configuration;
import org.mockserver.integration.ClientAndServer;
@@ -614,6 +616,24 @@ public class TestHTTPClient {
assertThat(responseHeaders.get().get("etag")).isEqualTo("W/\"1\"");
}
+ @ParameterizedTest
+ @NullSource
+ @ValueSource(strings = {"PT9999999999999H", "PT99999999999999999999H",
"garbage", "30m", ""})
+ public void invalidIdempotencyKeyLifetimeIsIgnored(String value) {
+ assertThat(HTTPClient.parseKeyLifetime(value)).isNull();
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {"PT0S", "PT-5M", "-PT5M"})
+ public void nonPositiveIdempotencyKeyLifetimeReturnsZero(String value) {
+ assertThat(HTTPClient.parseKeyLifetime(value)).isEqualTo(Duration.ZERO);
+ }
+
+ @Test
+ public void validIdempotencyKeyLifetimeIsParsed() {
+
assertThat(HTTPClient.parseKeyLifetime("PT30M")).isEqualTo(Duration.ofMinutes(30));
+ }
+
public static void testHttpMethodOnSuccess(HttpMethod method) throws
JsonProcessingException {
Item body = new Item(0L, "hank");
int statusCode = 200;
