From fa566bd070731ddff6a2d66fd6b1cdda121eeb00 Mon Sep 17 00:00:00 2001 From: Prashant Singh Date: Wed, 16 Apr 2025 10:24:44 -0700 Subject: [PATCH 1/9] REST: Avoid table corruption on unhandled RestException --- .../iceberg/rest/RESTTableOperations.java | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java b/core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java index 5f6c28b32337..347abc3db3e9 100644 --- a/core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java +++ b/core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java @@ -18,6 +18,7 @@ */ package org.apache.iceberg.rest; +import java.io.IOException; import java.util.List; import java.util.Map; import java.util.Objects; @@ -32,6 +33,8 @@ import org.apache.iceberg.UpdateRequirement; import org.apache.iceberg.UpdateRequirements; import org.apache.iceberg.encryption.EncryptionManager; +import org.apache.iceberg.exceptions.CommitStateUnknownException; +import org.apache.iceberg.exceptions.RESTException; import org.apache.iceberg.io.FileIO; import org.apache.iceberg.io.LocationProvider; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; @@ -155,13 +158,21 @@ public void commit(TableMetadata base, TableMetadata metadata) { // the error handler will throw necessary exceptions like CommitFailedException and // UnknownCommitStateException // TODO: ensure that the HTTP client lib passes HTTP client errors to the error handler - LoadTableResponse response = - client.post(path, request, LoadTableResponse.class, headers, errorHandler); + try { + LoadTableResponse response = + client.post(path, request, LoadTableResponse.class, headers, errorHandler); + // all future commits should be simple commits + this.updateType = UpdateType.SIMPLE; - // all future commits should be simple commits - this.updateType = UpdateType.SIMPLE; - - updateCurrentMetadata(response); + updateCurrentMetadata(response); + } catch (RESTException e) { + if (e.getCause() != null && e.getCause() instanceof IOException) { + // any IOException or unhandled Exception should be considered as commit unknown + // so that caller can attempt to potentially reconcile + throw new CommitStateUnknownException(e); + } + throw e; + } } @Override From fff056dbd861affe4899ded758455c5d7b36167f Mon Sep 17 00:00:00 2001 From: Prashant Singh Date: Fri, 23 May 2025 20:45:12 -0700 Subject: [PATCH 2/9] Add actual fix --- .../apache/iceberg/rest/ErrorHandlers.java | 14 ++++++++++- .../ExponentialHttpRequestRetryStrategy.java | 13 ++++++++-- .../org/apache/iceberg/rest/HTTPClient.java | 25 +++++++++++++++---- .../iceberg/rest/RESTTableOperations.java | 23 +++++------------ .../iceberg/rest/responses/ErrorResponse.java | 17 +++++++++++-- 5 files changed, 65 insertions(+), 27 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java b/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java index 0fca2a5bc9dd..5e6958294a3f 100644 --- a/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java +++ b/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java @@ -91,7 +91,19 @@ public void accept(ErrorResponse error) { case 404: throw new NoSuchTableException("%s", error.message()); case 409: - throw new CommitFailedException("Commit failed: %s", error.message()); + if (error.isRetried()) { + // If the request was retried, it could probably also mean that internal retries + // happened + // which could have let the commit succeed at persistence but since the request is + // retried + // without rebase this would not be apt to do a clean-up for. + throw new CommitStateUnknownException( + new ServiceFailureException( + "Service failed: %s: %s", error.code(), error.message())); + } else { + throw new CommitFailedException("Commit failed: %s", error.message()); + } + case 500: case 502: case 504: 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 263b3c305af0..12ec8ed40029 100644 --- a/core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java +++ b/core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java @@ -97,6 +97,7 @@ class ExponentialHttpRequestRetryStrategy implements HttpRequestRetryStrategy { @Override public boolean retryRequest( HttpRequest request, IOException exception, int execCount, HttpContext context) { + context.setAttribute("is-retried", Boolean.TRUE); if (execCount > maxRetries) { // Do not retry if over max retries return false; @@ -118,12 +119,20 @@ public boolean retryRequest( } // Retry if the request is considered idempotent - return Method.isIdempotent(request.getMethod()); + boolean shouldRetry = Method.isIdempotent(request.getMethod()); + if (shouldRetry) { + context.setAttribute("is-retried", Boolean.TRUE); + } + return shouldRetry; } @Override public boolean retryRequest(HttpResponse response, int execCount, HttpContext context) { - return execCount <= maxRetries && retriableCodes.contains(response.getCode()); + boolean shouldRetry = execCount <= maxRetries && retriableCodes.contains(response.getCode()); + if (shouldRetry) { + context.setAttribute("is-retried", Boolean.TRUE); + } + return shouldRetry; } @Override 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 532fff240141..93aa18030430 100644 --- a/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java +++ b/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java @@ -48,6 +48,8 @@ import org.apache.hc.core5.http.impl.EnglishReasonPhraseCatalog; import org.apache.hc.core5.http.io.entity.EntityUtils; import org.apache.hc.core5.http.io.entity.StringEntity; +import org.apache.hc.core5.http.protocol.BasicHttpContext; +import org.apache.hc.core5.http.protocol.HttpContext; import org.apache.hc.core5.io.CloseMode; import org.apache.iceberg.IcebergBuild; import org.apache.iceberg.exceptions.RESTException; @@ -183,7 +185,10 @@ private static ErrorResponse buildDefaultErrorResponse(CloseableHttpResponse res // Process a failed response through the provided errorHandler, and throw a RESTException if the // provided error handler doesn't already throw. private static void throwFailure( - CloseableHttpResponse response, String responseBody, Consumer errorHandler) { + CloseableHttpResponse response, + String responseBody, + Consumer errorHandler, + Object isRetried) { ErrorResponse errorResponse = null; if (responseBody != null) { @@ -220,10 +225,19 @@ private static void throwFailure( errorResponse = buildDefaultErrorResponse(response); } - errorHandler.accept(errorResponse); + boolean hasBeenRetried = (isRetried == Boolean.TRUE); + ErrorResponse enrichedErrorResponse = + ErrorResponse.builder() + .isRetried(hasBeenRetried) + .responseCode(errorResponse.code()) + .withMessage(errorResponse.message()) + .withType(errorResponse.type()) + .build(); + + errorHandler.accept(enrichedErrorResponse); // Throw an exception in case the provided error handler does not throw. - throw new RESTException("Unhandled error: %s", errorResponse); + throw new RESTException("Unhandled error: %s", enrichedErrorResponse); } @Override @@ -286,7 +300,8 @@ protected T execute( request.setEntity(new StringEntity(encodedBody)); } - try (CloseableHttpResponse response = httpClient.execute(request)) { + HttpContext context = new BasicHttpContext(); + try (CloseableHttpResponse response = httpClient.execute(request, context)) { Map respHeaders = Maps.newHashMap(); for (Header header : response.getHeaders()) { respHeaders.put(header.getName(), header.getValue()); @@ -304,7 +319,7 @@ protected T execute( if (!isSuccessful(response)) { // The provided error handler is expected to throw, but a RESTException is thrown if not. - throwFailure(response, responseBody, errorHandler); + throwFailure(response, responseBody, errorHandler, context.getAttribute("is-retried")); } if (responseBody == null) { diff --git a/core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java b/core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java index 347abc3db3e9..5f6c28b32337 100644 --- a/core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java +++ b/core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java @@ -18,7 +18,6 @@ */ package org.apache.iceberg.rest; -import java.io.IOException; import java.util.List; import java.util.Map; import java.util.Objects; @@ -33,8 +32,6 @@ import org.apache.iceberg.UpdateRequirement; import org.apache.iceberg.UpdateRequirements; import org.apache.iceberg.encryption.EncryptionManager; -import org.apache.iceberg.exceptions.CommitStateUnknownException; -import org.apache.iceberg.exceptions.RESTException; import org.apache.iceberg.io.FileIO; import org.apache.iceberg.io.LocationProvider; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; @@ -158,21 +155,13 @@ public void commit(TableMetadata base, TableMetadata metadata) { // the error handler will throw necessary exceptions like CommitFailedException and // UnknownCommitStateException // TODO: ensure that the HTTP client lib passes HTTP client errors to the error handler - try { - LoadTableResponse response = - client.post(path, request, LoadTableResponse.class, headers, errorHandler); - // all future commits should be simple commits - this.updateType = UpdateType.SIMPLE; + LoadTableResponse response = + client.post(path, request, LoadTableResponse.class, headers, errorHandler); - updateCurrentMetadata(response); - } catch (RESTException e) { - if (e.getCause() != null && e.getCause() instanceof IOException) { - // any IOException or unhandled Exception should be considered as commit unknown - // so that caller can attempt to potentially reconcile - throw new CommitStateUnknownException(e); - } - throw e; - } + // all future commits should be simple commits + this.updateType = UpdateType.SIMPLE; + + updateCurrentMetadata(response); } @Override diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java b/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java index da93b821cedd..940559484bb6 100644 --- a/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java +++ b/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java @@ -32,12 +32,15 @@ public class ErrorResponse implements RESTResponse { private final String type; private final int code; private final List stack; + private boolean isRetried; - private ErrorResponse(String message, String type, int code, List stack) { + private ErrorResponse( + String message, String type, int code, List stack, boolean isRetried) { this.message = message; this.type = type; this.code = code; this.stack = stack; + this.isRetried = isRetried; validate(); } @@ -62,6 +65,10 @@ public List stack() { return stack; } + public boolean isRetried() { + return true; + } + @Override public String toString() { StringBuilder sb = new StringBuilder(); @@ -92,6 +99,7 @@ public static class Builder { private String type; private Integer code; private List stack; + private boolean isRetried; private Builder() {} @@ -126,9 +134,14 @@ public Builder responseCode(Integer responseCode) { return this; } + public Builder isRetried(Boolean isRetried) { + this.isRetried = isRetried; + return this; + } + public ErrorResponse build() { Preconditions.checkArgument(code != null, "Invalid response, missing field: code"); - return new ErrorResponse(message, type, code, stack); + return new ErrorResponse(message, type, code, stack, isRetried); } } } From 45d0680ef200ec29683e4d8e5b56a7d215a7b1c3 Mon Sep 17 00:00:00 2001 From: Prashant Singh Date: Fri, 23 May 2025 21:05:47 -0700 Subject: [PATCH 3/9] Add unit test --- .../rest/ExponentialHttpRequestRetryStrategy.java | 1 - .../iceberg/rest/responses/ErrorResponse.java | 4 ++-- .../TestExponentialHttpRequestRetryStrategy.java | 14 +++++++++++--- 3 files changed, 13 insertions(+), 6 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 12ec8ed40029..d90fb3b4fb25 100644 --- a/core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java +++ b/core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java @@ -97,7 +97,6 @@ class ExponentialHttpRequestRetryStrategy implements HttpRequestRetryStrategy { @Override public boolean retryRequest( HttpRequest request, IOException exception, int execCount, HttpContext context) { - context.setAttribute("is-retried", Boolean.TRUE); if (execCount > maxRetries) { // Do not retry if over max retries return false; diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java b/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java index 940559484bb6..4d07614ca7be 100644 --- a/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java +++ b/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java @@ -32,7 +32,7 @@ public class ErrorResponse implements RESTResponse { private final String type; private final int code; private final List stack; - private boolean isRetried; + private final boolean isRetried; private ErrorResponse( String message, String type, int code, List stack, boolean isRetried) { @@ -66,7 +66,7 @@ public List stack() { } public boolean isRetried() { - return true; + return isRetried; } @Override 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 7d8c58701a1e..a29017c6e04f 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestExponentialHttpRequestRetryStrategy.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestExponentialHttpRequestRetryStrategy.java @@ -37,6 +37,8 @@ import org.apache.hc.core5.http.HttpHeaders; import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.message.BasicHttpResponse; +import org.apache.hc.core5.http.protocol.BasicHttpContext; +import org.apache.hc.core5.http.protocol.HttpContext; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -80,14 +82,20 @@ public void exponentialRetry() { @Test public void basicRetry() { + HttpContext context503 = new BasicHttpContext(); BasicHttpResponse response503 = new BasicHttpResponse(503, "Oopsie"); - assertThat(retryStrategy.retryRequest(response503, 3, null)).isTrue(); + assertThat(retryStrategy.retryRequest(response503, 3, context503)).isTrue(); + assertThat(context503.getAttribute("is-retried") == Boolean.TRUE).isTrue(); + HttpContext context429 = new BasicHttpContext(); BasicHttpResponse response429 = new BasicHttpResponse(429, "Oopsie"); - assertThat(retryStrategy.retryRequest(response429, 3, null)).isTrue(); + assertThat(retryStrategy.retryRequest(response429, 3, context429)).isTrue(); + assertThat(context429.getAttribute("is-retried") == Boolean.TRUE).isTrue(); + HttpContext context404 = new BasicHttpContext(); BasicHttpResponse response404 = new BasicHttpResponse(404, "Oopsie"); - assertThat(retryStrategy.retryRequest(response404, 3, null)).isFalse(); + assertThat(retryStrategy.retryRequest(response404, 3, context404)).isFalse(); + assertThat(context429.getAttribute("is-retried") == Boolean.TRUE).isTrue(); } @Test From 7c4dd8876809dbe62304c205de43c1a37ae4b193 Mon Sep 17 00:00:00 2001 From: Prashant Singh Date: Fri, 23 May 2025 21:12:55 -0700 Subject: [PATCH 4/9] Add more comment --- .../java/org/apache/iceberg/rest/ErrorHandlers.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java b/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java index 5e6958294a3f..3b1a61eb904e 100644 --- a/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java +++ b/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java @@ -92,11 +92,11 @@ public void accept(ErrorResponse error) { throw new NoSuchTableException("%s", error.message()); case 409: if (error.isRetried()) { - // If the request was retried, it could probably also mean that internal retries - // happened - // which could have let the commit succeed at persistence but since the request is - // retried - // without rebase this would not be apt to do a clean-up for. + // If the request was retried, and if it was 409, it could probably also mean that + // HTTP retries, and the IRC service would have actually applied the commit in the + // persistence. This would now mean since the base has changed it would conflict with + // itself, in cases like this its best not to mark this as failed instead make this + // is a commit state unknown. throw new CommitStateUnknownException( new ServiceFailureException( "Service failed: %s: %s", error.code(), error.message())); From 5bd0567c0134cc9d137f3198afd3f18c9b961ec9 Mon Sep 17 00:00:00 2001 From: Prashant Singh Date: Sat, 24 May 2025 08:50:54 -0700 Subject: [PATCH 5/9] fix lint --- .../org/apache/iceberg/rest/ErrorHandlers.java | 16 +++++++++------- .../ExponentialHttpRequestRetryStrategy.java | 4 ++-- .../java/org/apache/iceberg/rest/HTTPClient.java | 1 + .../iceberg/rest/responses/ErrorResponse.java | 4 ++-- .../TestExponentialHttpRequestRetryStrategy.java | 9 ++++++--- 5 files changed, 20 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java b/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java index 3b1a61eb904e..f03b3acbc78e 100644 --- a/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java +++ b/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java @@ -92,14 +92,16 @@ public void accept(ErrorResponse error) { throw new NoSuchTableException("%s", error.message()); case 409: if (error.isRetried()) { - // If the request was retried, and if it was 409, it could probably also mean that - // HTTP retries, and the IRC service would have actually applied the commit in the - // persistence. This would now mean since the base has changed it would conflict with - // itself, in cases like this its best not to mark this as failed instead make this - // is a commit state unknown. + // If the request was retried, and if it final error was 409, it could probably also + // mean that HTTP retries happened, and the IRC service would have actually applied + // the commit in the persistence during the retries by still gave back 5xx. + // This would now mean since the base has changed it would conflict with itself, + // in cases like this its best not to mark this as failed instead make this is + // a commit state unknown. throw new CommitStateUnknownException( - new ServiceFailureException( - "Service failed: %s: %s", error.code(), error.message())); + new RESTException( + "Commit status unknown, due to retries: %s: %s", + error.code(), error.message())); } else { throw new CommitFailedException("Commit failed: %s", error.message()); } 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 d90fb3b4fb25..66880d332c72 100644 --- a/core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java +++ b/core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java @@ -119,7 +119,7 @@ public boolean retryRequest( // Retry if the request is considered idempotent boolean shouldRetry = Method.isIdempotent(request.getMethod()); - if (shouldRetry) { + if (shouldRetry && context != null) { context.setAttribute("is-retried", Boolean.TRUE); } return shouldRetry; @@ -128,7 +128,7 @@ public boolean retryRequest( @Override public boolean retryRequest(HttpResponse response, int execCount, HttpContext context) { boolean shouldRetry = execCount <= maxRetries && retriableCodes.contains(response.getCode()); - if (shouldRetry) { + if (shouldRetry && context != null) { context.setAttribute("is-retried", Boolean.TRUE); } return shouldRetry; 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 93aa18030430..8a0f93bc4311 100644 --- a/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java +++ b/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java @@ -232,6 +232,7 @@ private static void throwFailure( .responseCode(errorResponse.code()) .withMessage(errorResponse.message()) .withType(errorResponse.type()) + .withStackTrace(errorResponse.stack()) .build(); errorHandler.accept(enrichedErrorResponse); diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java b/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java index 4d07614ca7be..3e92ff41140b 100644 --- a/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java +++ b/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java @@ -134,8 +134,8 @@ public Builder responseCode(Integer responseCode) { return this; } - public Builder isRetried(Boolean isRetried) { - this.isRetried = isRetried; + public Builder isRetried(Boolean hasBeenRetried) { + this.isRetried = hasBeenRetried; return this; } 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 a29017c6e04f..fcb4b1dbdbec 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestExponentialHttpRequestRetryStrategy.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestExponentialHttpRequestRetryStrategy.java @@ -163,8 +163,9 @@ public void noRetryOnAbortedRequests() { @Test public void retryOnNonAbortedRequests() { HttpGet request = new HttpGet("/"); + HttpContext context = new BasicHttpContext(); - assertThat(retryStrategy.retryRequest(request, new IOException(), 1, null)).isTrue(); + assertThat(retryStrategy.retryRequest(request, new IOException(), 1, context)).isTrue(); } @Test @@ -207,13 +208,15 @@ public void invalidRetryAfterHeader() { @Test public void testRetryBadGateway() { + HttpContext context = new BasicHttpContext(); BasicHttpResponse response502 = new BasicHttpResponse(502, "Bad gateway failure"); - assertThat(retryStrategy.retryRequest(response502, 3, null)).isTrue(); + assertThat(retryStrategy.retryRequest(response502, 3, context)).isTrue(); } @Test public void testRetryGatewayTimeout() { + HttpContext context = new BasicHttpContext(); BasicHttpResponse response504 = new BasicHttpResponse(504, "Gateway timeout"); - assertThat(retryStrategy.retryRequest(response504, 3, null)).isTrue(); + assertThat(retryStrategy.retryRequest(response504, 3, context)).isTrue(); } } From 82e6dfaee2fd17c8b0904cefdd5a1c034e349aa8 Mon Sep 17 00:00:00 2001 From: Prashant Singh Date: Sat, 24 May 2025 17:01:16 -0700 Subject: [PATCH 6/9] update wording --- .../java/org/apache/iceberg/rest/ErrorHandlers.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java b/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java index f03b3acbc78e..10a05fb322b5 100644 --- a/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java +++ b/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java @@ -93,11 +93,12 @@ public void accept(ErrorResponse error) { case 409: if (error.isRetried()) { // If the request was retried, and if it final error was 409, it could probably also - // mean that HTTP retries happened, and the IRC service would have actually applied - // the commit in the persistence during the retries by still gave back 5xx. - // This would now mean since the base has changed it would conflict with itself, - // in cases like this its best not to mark this as failed instead make this is - // a commit state unknown. + // mean that HTTP retries when happened the IRC service could have actually applied + // the commit in their persistence, while giving back the client still a 5xx. + // If so, since the base has changed, it could conflict with itself. + // In cases like this its best not to mark this as failed instead + // make this is a commit state unknown, so some reconciliation can happen and + // the metadata clean-up is not triggered. throw new CommitStateUnknownException( new RESTException( "Commit status unknown, due to retries: %s: %s", From afe2ee553b2c2e572c9e7d91efa3c2682fe105cc Mon Sep 17 00:00:00 2001 From: Prashant Singh Date: Tue, 27 May 2025 12:00:43 -0700 Subject: [PATCH 7/9] Address review feedback --- .../apache/iceberg/rest/ErrorHandlers.java | 4 ++-- .../ExponentialHttpRequestRetryStrategy.java | 4 ++-- .../org/apache/iceberg/rest/HTTPClient.java | 7 +++---- .../iceberg/rest/responses/ErrorResponse.java | 18 ++++++++--------- ...stExponentialHttpRequestRetryStrategy.java | 6 +++--- .../apache/iceberg/rest/TestRESTCatalog.java | 20 +++++++++++++++++++ 6 files changed, 39 insertions(+), 20 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java b/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java index 10a05fb322b5..ff08a346fa4a 100644 --- a/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java +++ b/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java @@ -91,8 +91,8 @@ public void accept(ErrorResponse error) { case 404: throw new NoSuchTableException("%s", error.message()); case 409: - if (error.isRetried()) { - // If the request was retried, and if it final error was 409, it could probably also + if (error.wasRetried()) { + // If the request was retried, and if its final error was 409, It could probably also // mean that HTTP retries when happened the IRC service could have actually applied // the commit in their persistence, while giving back the client still a 5xx. // If so, since the base has changed, it could conflict with itself. 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 66880d332c72..bbda16e6aa4b 100644 --- a/core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java +++ b/core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java @@ -120,7 +120,7 @@ public boolean retryRequest( // Retry if the request is considered idempotent boolean shouldRetry = Method.isIdempotent(request.getMethod()); if (shouldRetry && context != null) { - context.setAttribute("is-retried", Boolean.TRUE); + context.setAttribute("was-retried", Boolean.TRUE); } return shouldRetry; } @@ -129,7 +129,7 @@ public boolean retryRequest( public boolean retryRequest(HttpResponse response, int execCount, HttpContext context) { boolean shouldRetry = execCount <= maxRetries && retriableCodes.contains(response.getCode()); if (shouldRetry && context != null) { - context.setAttribute("is-retried", Boolean.TRUE); + context.setAttribute("was-retried", Boolean.TRUE); } return shouldRetry; } 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 8a0f93bc4311..bd301bc4f2ce 100644 --- a/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java +++ b/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java @@ -188,7 +188,7 @@ private static void throwFailure( CloseableHttpResponse response, String responseBody, Consumer errorHandler, - Object isRetried) { + Object wasRetried) { ErrorResponse errorResponse = null; if (responseBody != null) { @@ -225,10 +225,9 @@ private static void throwFailure( errorResponse = buildDefaultErrorResponse(response); } - boolean hasBeenRetried = (isRetried == Boolean.TRUE); ErrorResponse enrichedErrorResponse = ErrorResponse.builder() - .isRetried(hasBeenRetried) + .wasRetried(wasRetried == Boolean.TRUE) .responseCode(errorResponse.code()) .withMessage(errorResponse.message()) .withType(errorResponse.type()) @@ -320,7 +319,7 @@ protected T execute( if (!isSuccessful(response)) { // The provided error handler is expected to throw, but a RESTException is thrown if not. - throwFailure(response, responseBody, errorHandler, context.getAttribute("is-retried")); + throwFailure(response, responseBody, errorHandler, context.getAttribute("was-retried")); } if (responseBody == null) { diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java b/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java index 3e92ff41140b..c1e83c68033e 100644 --- a/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java +++ b/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java @@ -32,15 +32,15 @@ public class ErrorResponse implements RESTResponse { private final String type; private final int code; private final List stack; - private final boolean isRetried; + private final boolean wasRetried; private ErrorResponse( - String message, String type, int code, List stack, boolean isRetried) { + String message, String type, int code, List stack, boolean wasRetried) { this.message = message; this.type = type; this.code = code; this.stack = stack; - this.isRetried = isRetried; + this.wasRetried = wasRetried; validate(); } @@ -65,8 +65,8 @@ public List stack() { return stack; } - public boolean isRetried() { - return isRetried; + public boolean wasRetried() { + return wasRetried; } @Override @@ -99,7 +99,7 @@ public static class Builder { private String type; private Integer code; private List stack; - private boolean isRetried; + private boolean wasRetried; private Builder() {} @@ -134,14 +134,14 @@ public Builder responseCode(Integer responseCode) { return this; } - public Builder isRetried(Boolean hasBeenRetried) { - this.isRetried = hasBeenRetried; + public Builder wasRetried(Boolean hasBeenRetried) { + this.wasRetried = hasBeenRetried; return this; } public ErrorResponse build() { Preconditions.checkArgument(code != null, "Invalid response, missing field: code"); - return new ErrorResponse(message, type, code, stack, isRetried); + return new ErrorResponse(message, type, code, stack, wasRetried); } } } 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 fcb4b1dbdbec..00a4efa1f956 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestExponentialHttpRequestRetryStrategy.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestExponentialHttpRequestRetryStrategy.java @@ -85,17 +85,17 @@ public void basicRetry() { HttpContext context503 = new BasicHttpContext(); BasicHttpResponse response503 = new BasicHttpResponse(503, "Oopsie"); assertThat(retryStrategy.retryRequest(response503, 3, context503)).isTrue(); - assertThat(context503.getAttribute("is-retried") == Boolean.TRUE).isTrue(); + assertThat(context503.getAttribute("was-retried") == Boolean.TRUE).isTrue(); HttpContext context429 = new BasicHttpContext(); BasicHttpResponse response429 = new BasicHttpResponse(429, "Oopsie"); assertThat(retryStrategy.retryRequest(response429, 3, context429)).isTrue(); - assertThat(context429.getAttribute("is-retried") == Boolean.TRUE).isTrue(); + assertThat(context429.getAttribute("was-retried") == Boolean.TRUE).isTrue(); HttpContext context404 = new BasicHttpContext(); BasicHttpResponse response404 = new BasicHttpResponse(404, "Oopsie"); assertThat(retryStrategy.retryRequest(response404, 3, context404)).isFalse(); - assertThat(context429.getAttribute("is-retried") == Boolean.TRUE).isTrue(); + assertThat(context429.getAttribute("was-retried") == Boolean.TRUE).isTrue(); } @Test diff --git a/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java b/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java index 30b1ddef5b17..df199959c553 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java @@ -64,6 +64,7 @@ import org.apache.iceberg.catalog.TableCommit; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.exceptions.CommitFailedException; +import org.apache.iceberg.exceptions.CommitStateUnknownException; import org.apache.iceberg.exceptions.NotAuthorizedException; import org.apache.iceberg.exceptions.NotFoundException; import org.apache.iceberg.exceptions.ServiceFailureException; @@ -94,6 +95,7 @@ import org.eclipse.jetty.servlet.ServletContextHandler; import org.eclipse.jetty.servlet.ServletHolder; import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -2650,6 +2652,24 @@ public void testTableExistsFallbackToGETRequest() { ConfigResponse.builder().withEndpoints(ImmutableList.of(Endpoint.V1_LOAD_TABLE)).build()); } + @Test + public void testErrorHandlingForConflicts() { + Consumer errorResponseConsumer = ErrorHandlers.tableCommitHandler(); + // server returning 409 with client without retrying + ErrorResponse errorResponse409WithoutRetries = + ErrorResponse.builder().responseCode(409).wasRetried(false).build(); + Assertions.assertThrows( + CommitFailedException.class, + () -> errorResponseConsumer.accept(errorResponse409WithoutRetries)); + + // server returning 409, with retries. + ErrorResponse errorResponse409WithRetries = + ErrorResponse.builder().responseCode(409).wasRetried(true).build(); + Assertions.assertThrows( + CommitStateUnknownException.class, + () -> errorResponseConsumer.accept(errorResponse409WithRetries)); + } + private void verifyTableExistsFallbackToGETRequest(ConfigResponse configResponse) { RESTCatalogAdapter adapter = Mockito.spy( From 9228288f3c2125be8bd6551b0146bd95d806ea63 Mon Sep 17 00:00:00 2001 From: Prashant Singh Date: Tue, 27 May 2025 22:32:27 -0700 Subject: [PATCH 8/9] Address review feedback --- .../java/org/apache/iceberg/rest/responses/ErrorResponse.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java b/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java index c1e83c68033e..e9a7b0f27574 100644 --- a/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java +++ b/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java @@ -134,7 +134,7 @@ public Builder responseCode(Integer responseCode) { return this; } - public Builder wasRetried(Boolean hasBeenRetried) { + public Builder wasRetried(boolean hasBeenRetried) { this.wasRetried = hasBeenRetried; return this; } From 5f651ec6fb212e0775c70cc8f2769660ab4328b9 Mon Sep 17 00:00:00 2001 From: Prashant Singh Date: Wed, 28 May 2025 20:42:21 -0700 Subject: [PATCH 9/9] Address review feedback --- .../java/org/apache/iceberg/rest/ErrorHandlers.java | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java b/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java index ff08a346fa4a..acc3439a6ca7 100644 --- a/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java +++ b/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java @@ -92,13 +92,11 @@ public void accept(ErrorResponse error) { throw new NoSuchTableException("%s", error.message()); case 409: if (error.wasRetried()) { - // If the request was retried, and if its final error was 409, It could probably also - // mean that HTTP retries when happened the IRC service could have actually applied - // the commit in their persistence, while giving back the client still a 5xx. - // If so, since the base has changed, it could conflict with itself. - // In cases like this its best not to mark this as failed instead - // make this is a commit state unknown, so some reconciliation can happen and - // the metadata clean-up is not triggered. + // If a retried request finally fails with 409, + // the IRC service may have persisted the commit + // despite initial 5xx errors, resulting in a self-conflict on retry + // due to the base changing. + // Mark this failure as commit state unknown rather than failed to prevent file cleanup. throw new CommitStateUnknownException( new RESTException( "Commit status unknown, due to retries: %s: %s", @@ -106,7 +104,6 @@ public void accept(ErrorResponse error) { } else { throw new CommitFailedException("Commit failed: %s", error.message()); } - case 500: case 502: case 504: