From f65d1deb7a6641f5ae6af8a7a84bfde2dfb60dce Mon Sep 17 00:00:00 2001 From: Alessandro Nori Date: Wed, 14 Jan 2026 18:16:06 +0100 Subject: [PATCH 1/6] use tableCommitHandler for create transactions --- .../main/java/org/apache/iceberg/rest/RESTTableOperations.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 d2a6ab618ca8..a4c42bfc6a72 100644 --- a/core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java +++ b/core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java @@ -169,7 +169,7 @@ public void commit(TableMetadata base, TableMetadata metadata) { .addAll(metadata.changes()) .build(); requirements = UpdateRequirements.forCreateTable(updates); - errorHandler = ErrorHandlers.tableErrorHandler(); // throws NoSuchTableException + errorHandler = ErrorHandlers.tableCommitHandler(); // throws NoSuchTableException break; case REPLACE: From 0b83a6359d6af1bf7fbacb62ec819c5c47dee404 Mon Sep 17 00:00:00 2001 From: Alessandro Nori Date: Tue, 20 Jan 2026 22:45:50 +0100 Subject: [PATCH 2/6] add a new createTableErrorHanler --- .../apache/iceberg/rest/ErrorHandlers.java | 24 +++++++ .../iceberg/rest/RESTTableOperations.java | 2 +- .../apache/iceberg/rest/TestRESTCatalog.java | 68 +++++++++++++++++++ 3 files changed, 93 insertions(+), 1 deletion(-) 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 543e548529dd..02565120b73f 100644 --- a/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java +++ b/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java @@ -76,6 +76,10 @@ public static Consumer tableCommitHandler() { return CommitErrorHandler.INSTANCE; } + public static Consumer createTableErrorHandler() { + return CreateTableErrorHandler.INSTANCE; + } + public static Consumer planErrorHandler() { return PlanErrorHandler.INSTANCE; } @@ -138,6 +142,26 @@ public void accept(ErrorResponse error) { } } + /** Table create error handler */ + private static class CreateTableErrorHandler extends TableErrorHandler { + private static final ErrorHandler INSTANCE = new CreateTableErrorHandler(); + + @Override + public void accept(ErrorResponse error) { + switch (error.code()) { + case 500: + case 502: + case 503: + case 504: + throw new CommitStateUnknownException( + new ServiceFailureException("Service failed: %s: %s", error.code(), error.message())); + } + + // Delegate to parent for 404, 409, and all other error codes + super.accept(error); + } + } + /** Plan level error handler. */ private static class PlanErrorHandler extends DefaultErrorHandler { private static final ErrorHandler INSTANCE = new PlanErrorHandler(); 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 a4c42bfc6a72..20b9ba6367d2 100644 --- a/core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java +++ b/core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java @@ -169,7 +169,7 @@ public void commit(TableMetadata base, TableMetadata metadata) { .addAll(metadata.changes()) .build(); requirements = UpdateRequirements.forCreateTable(updates); - errorHandler = ErrorHandlers.tableCommitHandler(); // throws NoSuchTableException + errorHandler = ErrorHandlers.createTableErrorHandler(); // throws NoSuchTableException break; case REPLACE: 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 d202680e5626..841e4f66334e 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java @@ -2543,6 +2543,74 @@ public void testNoCleanupForNonCleanableCreateTransaction() { }); } + @Test + public void testNoCleanup503MappedToCommitStateUnknownCreateTransaction() { + RESTCatalogAdapter adapter = + Mockito.spy( + new RESTCatalogAdapter(backendCatalog) { + @Override + protected T execute( + HTTPRequest request, + Class responseType, + Consumer errorHandler, + Consumer> responseHeaders) { + if (request.method() == HTTPMethod.POST && request.path().contains("some_table")) { + // Simulate a 503 Service Unavailable error + ErrorResponse error = + ErrorResponse.builder() + .responseCode(503) + .withMessage("Service unavailable") + .build(); + + errorHandler.accept(error); + throw new IllegalStateException("Error handler should have thrown"); + } + return super.execute(request, responseType, errorHandler, responseHeaders); + } + }); + + RESTCatalog catalog = catalog(adapter); + + if (requiresNamespaceCreate()) { + catalog.createNamespace(TABLE.namespace()); + } + + catalog.createTable(TABLE, SCHEMA); + TableIdentifier newTable = TableIdentifier.of(TABLE.namespace(), "some_table"); + + Transaction createTableTransaction = catalog.newCreateTableTransaction(newTable, SCHEMA); + createTableTransaction.newAppend().appendFile(FILE_A).commit(); + + // Verify that 503 is mapped to CommitStateUnknownException (not just ServiceFailureException) + assertThatThrownBy(createTableTransaction::commitTransaction) + .isInstanceOf(CommitStateUnknownException.class) + .cause() + .isInstanceOf(ServiceFailureException.class) + .hasMessageContaining("Service failed: 503"); + + // Verify files are NOT cleaned up (because commit state is unknown) + assertThat(allRequests(adapter)) + .anySatisfy( + req -> { + assertThat(req.method()).isEqualTo(HTTPMethod.POST); + assertThat(req.path()).isEqualTo(RESOURCE_PATHS.table(newTable)); + assertThat(req.body()).isInstanceOf(UpdateTableRequest.class); + UpdateTableRequest body = (UpdateTableRequest) req.body(); + Optional appendSnapshot = + body.updates().stream() + .filter(update -> update instanceof MetadataUpdate.AddSnapshot) + .findFirst(); + assertThat(appendSnapshot).isPresent(); + + MetadataUpdate.AddSnapshot addSnapshot = + (MetadataUpdate.AddSnapshot) appendSnapshot.get(); + String manifestListLocation = addSnapshot.snapshot().manifestListLocation(); + // Files should still exist because we don't know if commit succeeded + assertThat(catalog.loadTable(TABLE).io().newInputFile(manifestListLocation).exists()) + .isTrue(); + }); + } + @Test public void testCleanupCleanableExceptionsReplace() { RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog)); From 67e1a383b28b0c1f7fed5fdc16b49529fb85f827 Mon Sep 17 00:00:00 2001 From: Alessandro Nori Date: Fri, 23 Jan 2026 00:36:26 +0100 Subject: [PATCH 3/6] apply suggestion for test assertion --- .../apache/iceberg/rest/TestRESTCatalog.java | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) 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 841e4f66334e..90646bee2978 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java @@ -2596,18 +2596,23 @@ protected T execute( assertThat(req.path()).isEqualTo(RESOURCE_PATHS.table(newTable)); assertThat(req.body()).isInstanceOf(UpdateTableRequest.class); UpdateTableRequest body = (UpdateTableRequest) req.body(); - Optional appendSnapshot = - body.updates().stream() - .filter(update -> update instanceof MetadataUpdate.AddSnapshot) - .findFirst(); - assertThat(appendSnapshot).isPresent(); - - MetadataUpdate.AddSnapshot addSnapshot = - (MetadataUpdate.AddSnapshot) appendSnapshot.get(); - String manifestListLocation = addSnapshot.snapshot().manifestListLocation(); - // Files should still exist because we don't know if commit succeeded - assertThat(catalog.loadTable(TABLE).io().newInputFile(manifestListLocation).exists()) - .isTrue(); + assertThat( + body.updates().stream() + .filter(MetadataUpdate.AddSnapshot.class::isInstance) + .map(MetadataUpdate.AddSnapshot.class::cast) + .findFirst()) + .hasValueSatisfying( + addSnapshot -> { + String manifestListLocation = addSnapshot.snapshot().manifestListLocation(); + // Files should still exist because we don't know if commit succeeded + assertThat( + catalog + .loadTable(TABLE) + .io() + .newInputFile(manifestListLocation) + .exists()) + .isTrue(); + }); }); } From 26d4dda5f44b3910c42e53d3af2375a3fc8d37ad Mon Sep 17 00:00:00 2001 From: Alessandro Nori Date: Fri, 23 Jan 2026 00:46:21 +0100 Subject: [PATCH 4/6] createTableErrorHandler extends commitErrorHandler --- .../org/apache/iceberg/rest/ErrorHandlers.java | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 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 02565120b73f..be408816c0ba 100644 --- a/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java +++ b/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java @@ -142,22 +142,19 @@ public void accept(ErrorResponse error) { } } - /** Table create error handler */ - private static class CreateTableErrorHandler extends TableErrorHandler { + /** Table create error handler. */ + private static class CreateTableErrorHandler extends CommitErrorHandler { private static final ErrorHandler INSTANCE = new CreateTableErrorHandler(); @Override public void accept(ErrorResponse error) { switch (error.code()) { - case 500: - case 502: - case 503: - case 504: - throw new CommitStateUnknownException( - new ServiceFailureException("Service failed: %s: %s", error.code(), error.message())); + case 404: + throw new NoSuchNamespaceException("%s", error.message()); + case 409: + throw new AlreadyExistsException("%s", error.message()); } - // Delegate to parent for 404, 409, and all other error codes super.accept(error); } } From 428860fb95d004548a145be51fb9580e2c283816 Mon Sep 17 00:00:00 2001 From: Alessandro Nori Date: Fri, 30 Jan 2026 10:04:20 +0100 Subject: [PATCH 5/6] rename test --- core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 90646bee2978..86bec662d821 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java @@ -2544,7 +2544,7 @@ public void testNoCleanupForNonCleanableCreateTransaction() { } @Test - public void testNoCleanup503MappedToCommitStateUnknownCreateTransaction() { + public void testNoCleanupOnCreate503() { RESTCatalogAdapter adapter = Mockito.spy( new RESTCatalogAdapter(backendCatalog) { From 2a97fa6653391e9219e1e25c01fe0ccf5fb7d4dc Mon Sep 17 00:00:00 2001 From: Alessandro Nori Date: Mon, 2 Feb 2026 14:02:04 +0100 Subject: [PATCH 6/6] fix unit test --- .../org/apache/iceberg/rest/RESTTableOperations.java | 2 +- .../org/apache/iceberg/rest/TestRESTCatalog.java | 12 +++++------- 2 files changed, 6 insertions(+), 8 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 20b9ba6367d2..be763d30fef1 100644 --- a/core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java +++ b/core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java @@ -169,7 +169,7 @@ public void commit(TableMetadata base, TableMetadata metadata) { .addAll(metadata.changes()) .build(); requirements = UpdateRequirements.forCreateTable(updates); - errorHandler = ErrorHandlers.createTableErrorHandler(); // throws NoSuchTableException + errorHandler = ErrorHandlers.createTableErrorHandler(); break; case REPLACE: 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 86bec662d821..09543bd0f932 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java @@ -2554,7 +2554,8 @@ protected T execute( Class responseType, Consumer errorHandler, Consumer> responseHeaders) { - if (request.method() == HTTPMethod.POST && request.path().contains("some_table")) { + var response = super.execute(request, responseType, errorHandler, responseHeaders); + if (request.method() == HTTPMethod.POST && request.path().contains(TABLE.name())) { // Simulate a 503 Service Unavailable error ErrorResponse error = ErrorResponse.builder() @@ -2565,7 +2566,7 @@ protected T execute( errorHandler.accept(error); throw new IllegalStateException("Error handler should have thrown"); } - return super.execute(request, responseType, errorHandler, responseHeaders); + return response; } }); @@ -2575,10 +2576,7 @@ protected T execute( catalog.createNamespace(TABLE.namespace()); } - catalog.createTable(TABLE, SCHEMA); - TableIdentifier newTable = TableIdentifier.of(TABLE.namespace(), "some_table"); - - Transaction createTableTransaction = catalog.newCreateTableTransaction(newTable, SCHEMA); + Transaction createTableTransaction = catalog.newCreateTableTransaction(TABLE, SCHEMA); createTableTransaction.newAppend().appendFile(FILE_A).commit(); // Verify that 503 is mapped to CommitStateUnknownException (not just ServiceFailureException) @@ -2593,7 +2591,7 @@ protected T execute( .anySatisfy( req -> { assertThat(req.method()).isEqualTo(HTTPMethod.POST); - assertThat(req.path()).isEqualTo(RESOURCE_PATHS.table(newTable)); + assertThat(req.path()).isEqualTo(RESOURCE_PATHS.table(TABLE)); assertThat(req.body()).isInstanceOf(UpdateTableRequest.class); UpdateTableRequest body = (UpdateTableRequest) req.body(); assertThat(