REST: Revert apache#12818 and additionally stop retrying on 502/504 (apache#13352)

Author: singhpk234

core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java

@@ -91,19 +91,7 @@ public void accept(ErrorResponse error) {
         case 404:
           throw new NoSuchTableException("%s", error.message());
         case 409:
-          if (error.wasRetried()) {
-            // 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",
-                    error.code(), error.message()));
-          } else {
-            throw new CommitFailedException("Commit failed: %s", error.message());
-          }
+          throw new CommitFailedException("Commit failed: %s", error.message());
         case 500:
         case 502:
         case 504:

core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java

@@ -60,9 +60,7 @@
  *
  * <ul>
  *   <li>SC_TOO_MANY_REQUESTS (429)
- *   <li>SC_BAD_GATEWAY (502)
  *   <li>SC_SERVICE_UNAVAILABLE (503)
- *   <li>SC_GATEWAY_TIMEOUT (504)
  * </ul>
  *
  * Most code and behavior is taken from {@link
@@ -79,11 +77,7 @@ class ExponentialHttpRequestRetryStrategy implements HttpRequestRetryStrategy {
         maximumRetries > 0, "Cannot set retries to %s, the value must be positive", maximumRetries);
     this.maxRetries = maximumRetries;
     this.retriableCodes =
-        ImmutableSet.of(
-            HttpStatus.SC_TOO_MANY_REQUESTS,
-            HttpStatus.SC_SERVICE_UNAVAILABLE,
-            HttpStatus.SC_BAD_GATEWAY,
-            HttpStatus.SC_GATEWAY_TIMEOUT);
+        ImmutableSet.of(HttpStatus.SC_TOO_MANY_REQUESTS, HttpStatus.SC_SERVICE_UNAVAILABLE);
     this.nonRetriableExceptions =
         ImmutableSet.of(
             InterruptedIOException.class,
@@ -118,20 +112,12 @@ public boolean retryRequest(
     }
 
     // Retry if the request is considered idempotent
-    boolean shouldRetry = Method.isIdempotent(request.getMethod());
-    if (shouldRetry && context != null) {
-      context.setAttribute("was-retried", Boolean.TRUE);
-    }
-    return shouldRetry;
+    return Method.isIdempotent(request.getMethod());
   }
 
   @Override
   public boolean retryRequest(HttpResponse response, int execCount, HttpContext context) {
-    boolean shouldRetry = execCount <= maxRetries && retriableCodes.contains(response.getCode());
-    if (shouldRetry && context != null) {
-      context.setAttribute("was-retried", Boolean.TRUE);
-    }
-    return shouldRetry;
+    return execCount <= maxRetries && retriableCodes.contains(response.getCode());
   }
 
   @Override

core/src/main/java/org/apache/iceberg/rest/HTTPClient.java

@@ -45,8 +45,6 @@
 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;
@@ -177,10 +175,7 @@ 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<ErrorResponse> errorHandler,
-      Object wasRetried) {
+      CloseableHttpResponse response, String responseBody, Consumer<ErrorResponse> errorHandler) {
     ErrorResponse errorResponse = null;
 
     if (responseBody != null) {
@@ -217,19 +212,10 @@ private static void throwFailure(
       errorResponse = buildDefaultErrorResponse(response);
     }
 
-    ErrorResponse enrichedErrorResponse =
-        ErrorResponse.builder()
-            .wasRetried(wasRetried == Boolean.TRUE)
-            .responseCode(errorResponse.code())
-            .withMessage(errorResponse.message())
-            .withType(errorResponse.type())
-            .withStackTrace(errorResponse.stack())
-            .build();
-
-    errorHandler.accept(enrichedErrorResponse);
+    errorHandler.accept(errorResponse);
 
     // Throw an exception in case the provided error handler does not throw.
-    throw new RESTException("Unhandled error: %s", enrichedErrorResponse);
+    throw new RESTException("Unhandled error: %s", errorResponse);
   }
 
   @Override
@@ -292,8 +278,7 @@ protected <T extends RESTResponse> T execute(
       request.setEntity(new StringEntity(encodedBody));
     }
 
-    HttpContext context = new BasicHttpContext();
-    try (CloseableHttpResponse response = httpClient.execute(request, context)) {
+    try (CloseableHttpResponse response = httpClient.execute(request)) {
       Map<String, String> respHeaders = Maps.newHashMap();
       for (Header header : response.getHeaders()) {
         respHeaders.put(header.getName(), header.getValue());
@@ -311,7 +296,7 @@ protected <T extends RESTResponse> 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("was-retried"));
+        throwFailure(response, responseBody, errorHandler);
       }
 
       if (responseBody == null) {

core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java

@@ -32,15 +32,12 @@ public class ErrorResponse implements RESTResponse {
   private final String type;
   private final int code;
   private final List<String> stack;
-  private final boolean wasRetried;
 
-  private ErrorResponse(
-      String message, String type, int code, List<String> stack, boolean wasRetried) {
+  private ErrorResponse(String message, String type, int code, List<String> stack) {
     this.message = message;
     this.type = type;
     this.code = code;
     this.stack = stack;
-    this.wasRetried = wasRetried;
     validate();
   }
 
@@ -65,10 +62,6 @@ public List<String> stack() {
     return stack;
   }
 
-  public boolean wasRetried() {
-    return wasRetried;
-  }
-
   @Override
   public String toString() {
     StringBuilder sb = new StringBuilder();
@@ -99,7 +92,6 @@ public static class Builder {
     private String type;
     private Integer code;
     private List<String> stack;
-    private boolean wasRetried;
 
     private Builder() {}
 
@@ -134,14 +126,9 @@ public Builder responseCode(Integer responseCode) {
       return this;
     }
 
-    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, wasRetried);
+      return new ErrorResponse(message, type, code, stack);
     }
   }
 }

core/src/test/java/org/apache/iceberg/rest/TestExponentialHttpRequestRetryStrategy.java

@@ -37,8 +37,6 @@
 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;
@@ -82,20 +80,14 @@ public void exponentialRetry() {
 
   @Test
   public void basicRetry() {
-    HttpContext context503 = new BasicHttpContext();
     BasicHttpResponse response503 = new BasicHttpResponse(503, "Oopsie");
-    assertThat(retryStrategy.retryRequest(response503, 3, context503)).isTrue();
-    assertThat(context503.getAttribute("was-retried") == Boolean.TRUE).isTrue();
+    assertThat(retryStrategy.retryRequest(response503, 3, null)).isTrue();
 
-    HttpContext context429 = new BasicHttpContext();
     BasicHttpResponse response429 = new BasicHttpResponse(429, "Oopsie");
-    assertThat(retryStrategy.retryRequest(response429, 3, context429)).isTrue();
-    assertThat(context429.getAttribute("was-retried") == Boolean.TRUE).isTrue();
+    assertThat(retryStrategy.retryRequest(response429, 3, null)).isTrue();
 
-    HttpContext context404 = new BasicHttpContext();
     BasicHttpResponse response404 = new BasicHttpResponse(404, "Oopsie");
-    assertThat(retryStrategy.retryRequest(response404, 3, context404)).isFalse();
-    assertThat(context429.getAttribute("was-retried") == Boolean.TRUE).isTrue();
+    assertThat(retryStrategy.retryRequest(response404, 3, null)).isFalse();
   }
 
   @Test
@@ -163,9 +155,8 @@ public void noRetryOnAbortedRequests() {
   @Test
   public void retryOnNonAbortedRequests() {
     HttpGet request = new HttpGet("/");
-    HttpContext context = new BasicHttpContext();
 
-    assertThat(retryStrategy.retryRequest(request, new IOException(), 1, context)).isTrue();
+    assertThat(retryStrategy.retryRequest(request, new IOException(), 1, null)).isTrue();
   }
 
   @Test
@@ -206,17 +197,17 @@ public void invalidRetryAfterHeader() {
         .isBetween(4000L, 5000L);
   }
 
-  @Test
-  public void testRetryBadGateway() {
-    HttpContext context = new BasicHttpContext();
-    BasicHttpResponse response502 = new BasicHttpResponse(502, "Bad gateway failure");
-    assertThat(retryStrategy.retryRequest(response502, 3, context)).isTrue();
+  @ParameterizedTest
+  @ValueSource(ints = {429, 503})
+  public void testRetryHappensOnAcceptableStatusCodes(int statusCode) {
+    BasicHttpResponse response = new BasicHttpResponse(statusCode, String.valueOf(statusCode));
+    assertThat(retryStrategy.retryRequest(response, 3, null)).isTrue();
   }
 
-  @Test
-  public void testRetryGatewayTimeout() {
-    HttpContext context = new BasicHttpContext();
-    BasicHttpResponse response504 = new BasicHttpResponse(504, "Gateway timeout");
-    assertThat(retryStrategy.retryRequest(response504, 3, context)).isTrue();
+  @ParameterizedTest
+  @ValueSource(ints = {500, 502, 504})
+  public void testRetryDoesNotHappenOnUnacceptableStatusCodes(int statusCode) {
+    BasicHttpResponse response = new BasicHttpResponse(statusCode, String.valueOf(statusCode));
+    assertThat(retryStrategy.retryRequest(response, 3, null)).isFalse();
   }
 }

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java

@@ -62,7 +62,6 @@
 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;
@@ -93,7 +92,6 @@
 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,24 +2648,6 @@ public void testTableExistsFallbackToGETRequest() {
         ConfigResponse.builder().withEndpoints(ImmutableList.of(Endpoint.V1_LOAD_TABLE)).build());
   }
 
-  @Test
-  public void testErrorHandlingForConflicts() {
-    Consumer<ErrorResponse> 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(