can you please add some tests?

on a first glance this seems correct to me but you'll most likely also need to update

diff --git a/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java b/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java
index c2fd24856f..057a889bec 100644
--- a/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java
+++ b/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java
@@ -2182,8 +2182,10 @@ public abstract class CatalogTests<C extends Catalog & SupportsNamespaces> {
         supportsServerSideRetry()
             ? "Requirement failed: table already exists"
             : "Table already exists";
+    Class<? extends RuntimeException> expectedException =
+        supportsServerSideRetry() ? AlreadyExistsException.class : CommitFailedException.class;
     assertThatThrownBy(create::commitTransaction)
-        .isInstanceOf(AlreadyExistsException.class)
+        .isInstanceOf(expectedException)
         .hasMessageStartingWith(expectedMessage);

     // validate the concurrently created table is unmodified
@@ -2434,8 +2436,10 @@ public abstract class CatalogTests<C extends Catalog & SupportsNamespaces> {
         supportsServerSideRetry()
             ? "Requirement failed: table already exists"
             : "Table already exists";
+    Class<? extends RuntimeException> expectedException =
+        supportsServerSideRetry() ? AlreadyExistsException.class : CommitFailedException.class;
     assertThatThrownBy(createOrReplace::commitTransaction)
-        .isInstanceOf(AlreadyExistsException.class)
+        .isInstanceOf(expectedException)
         .hasMessageStartingWith(expectedMessage);

what do you think about 409?
I was not sure if in this case it was ok to return a CommitFailedException instead of a TableAlreadyExists exception

Yeah I think we may just want to add a createTableErrorHandler() which extends the commit error handler, but we override the 409 behavior to be a TableAlreadyExists exception, and then in any other case fallback to super.accept(error).

I feel like we should be able to slim this setup down a bit by just doing something like:

    RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
    Mockito.doThrow(new ServiceFailureException("some service failure"))
        .when(adapter)
        .execute(reqMatcher(HTTPMethod.POST), any(), any(), any());

Think it's OK to mock the commit state unknown here, since all we're trying to test is the client's reaction to it on create? I see the other tests in this class do that as well.

Thank you very much for your review.

My goal was to test that the new ErrorHandler is correctly mapping 503 errors to CommitStateUnknownException.

I tried debugging what you suggested but the ErrorHandler code is not called in that case. There is already a test to check that not CleanableFailures like ServiceFailureException do not delete files

My new test instead fails without the new error handler.

I don't have enough expertise in Java so it's very possible I'm doing something wrong.

Ah this is my bad, I had to look at the code further. You're right, given how the http client is invoked, to trigger a failure and go through the error handler, we can't just mock execute. The error handler invocation is handled below the execution API. I think this is fine, maybe we want a separate helper method for better simulation of errors that go through the error handlers.

On a separate note, I'm starting to think a bit more about how we look at CleanableFailure. The original intent of the CleanableFailure marker interface was to prevent issues in case arbitrary exceptions happened to be thrown on the commit path. We generally expect Error handlers to map to the commit state unknown, and any other exception we fail to handle goes through as a runtime exception, which we validate is cleanable or not. Anyways this is separate.

Minor point, I'd just shorten the test name a bit: testNoCleanupOnCreate503 or something. The commit state unknown is more of an internal handling detail for this case.

Maybe a more assertJ "fluent" way:

assertThat(body.updates().stream()
        .filter(MetadataUpdate.AddSnapshot.class::isInstance)
        .map(MetadataUpdate.AddSnapshot.class::cast)
        .findFirst())
    .hasValueSatisfying(addSnapshot -> {
      String manifestListLocation = addSnapshot.snapshot().manifestListLocation();
      assertThat(catalog.loadTable(TABLE).io().newInputFile(manifestListLocation).exists())
          .isTrue();
    });

This is also fine, just wondering it may be a smaller diff if we just extended CommitErrorHandler, and overrode the 409 case? Unless I'm missing a case in handling.

we also need to override 404 because it should be NoSuchNamespaceException for create-table instead of NoSuchTableException.

I like your approach better because CreateTableErrorHandler should be used in case of a commit.
I'm also fine with reverting 26d4dda in case we change our mind

Ah this is my bad, I had to look at the code further. You're right, given how the http client is invoked, to trigger a failure and go through the error handler, we can't just mock execute. The error handler invocation is handled below the execution API. I think this is fine, maybe we want a separate helper method for better simulation of errors that go through the error handlers.

On a separate note, I'm starting to think a bit more about how we look at CleanableFailure. The original intent of the CleanableFailure marker interface was to prevent issues in case arbitrary exceptions happened to be thrown on the commit path. We generally expect Error handlers to map to the commit state unknown, and any other exception we fail to handle goes through as a runtime exception, which we validate is cleanable or not. Anyways this is separate.

Minor point, I'd just shorten the test name a bit: testNoCleanupOnCreate503 or something. The commit state unknown is more of an internal handling detail for this case.