diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a75937528..222b9a825c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti ### Fixes - `RateLimiterFilter` now returns an Iceberg-compatible `ErrorResponse` JSON body on HTTP 429, with `Content-Type: application/json`. Previously the body was empty, causing Iceberg REST clients to surface an opaque error. - The admin tool `purge` command now prints the underlying exception stack trace to stderr when a purge fails unexpectedly, matching the `bootstrap` command. Previously a failed purge printed only a generic message, giving operators no diagnostic information. +- Renaming a table or view now maps concurrent-modification and resolution failures to meaningful HTTP status codes instead of HTTP 500. A concurrent modification of the source returns 503 (Service Unavailable, retryable); a source or target path that no longer resolves (concurrently dropped or replaced) returns 404 (Not Found). HTTP 409 is intentionally not used because the Iceberg REST rename endpoint reserves 409 for "the target already exists". ## [1.5.0] diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java index 239096cf4f..2b5867b0f4 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java @@ -74,6 +74,7 @@ import org.apache.iceberg.exceptions.NoSuchViewException; import org.apache.iceberg.exceptions.NotFoundException; import org.apache.iceberg.exceptions.ServiceFailureException; +import org.apache.iceberg.exceptions.ServiceUnavailableException; import org.apache.iceberg.exceptions.UnprocessableEntityException; import org.apache.iceberg.io.CloseableGroup; import org.apache.iceberg.io.FileIO; @@ -2494,10 +2495,22 @@ private void renameTableLike( case BaseResult.ReturnStatus.ENTITY_NOT_FOUND: throw new NotFoundException("Cannot rename %s to %s. %s does not exist", from, to, from); - // this is temporary. Should throw a special error that will be caught and retried - case BaseResult.ReturnStatus.TARGET_ENTITY_CONCURRENTLY_MODIFIED: + // The source path (ENTITY_CANNOT_BE_RESOLVED) or the target path + // (CATALOG_PATH_CANNOT_BE_RESOLVED) could not be resolved, e.g. because it was concurrently + // dropped. This is not retriable, so surface it as 404. case BaseResult.ReturnStatus.ENTITY_CANNOT_BE_RESOLVED: - throw new RuntimeException("concurrent update detected, please retry"); + case BaseResult.ReturnStatus.CATALOG_PATH_CANNOT_BE_RESOLVED: + throw new NoSuchNamespaceException( + "Cannot rename %s to %s because the source or target path could not be resolved", + from, to); + + // The entity is still present but was concurrently modified: a genuine transient conflict. + // Surface as 503 so clients can retry. We avoid 409 because the rename endpoint reserves + // 409 for "target already exists" (handled by the ENTITY_ALREADY_EXISTS case above). + case BaseResult.ReturnStatus.TARGET_ENTITY_CONCURRENTLY_MODIFIED: + throw new ServiceUnavailableException( + "Cannot rename %s to %s because it was concurrently modified; please retry", + from, to); // some entities cannot be renamed case BaseResult.ReturnStatus.ENTITY_CANNOT_BE_RENAMED: diff --git a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java index 3701206dd7..fb77eb89db 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java @@ -88,6 +88,7 @@ import org.apache.iceberg.exceptions.ForbiddenException; import org.apache.iceberg.exceptions.NoSuchNamespaceException; import org.apache.iceberg.exceptions.ServiceFailureException; +import org.apache.iceberg.exceptions.ServiceUnavailableException; import org.apache.iceberg.inmemory.InMemoryFileIO; import org.apache.iceberg.io.CloseableIterable; import org.apache.iceberg.io.FileIO; @@ -2529,6 +2530,57 @@ public void testConcurrencyConflictUpdateTableDuringFinalTransaction() { .hasMessageContaining("conflict_table"); } + static Stream renameFailureStatuses() { + return Stream.of( + // Transient conflict: entity present but concurrently modified -> 503, retryable. + Arguments.of( + BaseResult.ReturnStatus.TARGET_ENTITY_CONCURRENTLY_MODIFIED, + ServiceUnavailableException.class), + // Source path could not be resolved (e.g. concurrently dropped) -> 404, not retryable. + Arguments.of( + BaseResult.ReturnStatus.ENTITY_CANNOT_BE_RESOLVED, NoSuchNamespaceException.class), + // Target path could not be resolved (e.g. concurrently dropped) -> 404, not retryable. + Arguments.of( + BaseResult.ReturnStatus.CATALOG_PATH_CANNOT_BE_RESOLVED, + NoSuchNamespaceException.class)); + } + + @ParameterizedTest + @MethodSource("renameFailureStatuses") + public void testConcurrencyConflictRenameTable( + BaseResult.ReturnStatus renameStatus, Class expectedException) { + Assumptions.assumeTrue( + requiresNamespaceCreate(), + "Only applicable if namespaces must be created before adding children"); + + // Use a spy so that resolution succeeds normally, but the final rename reports the given + // failure status. The mapping must distinguish a transient conflict + // (TARGET_ENTITY_CONCURRENTLY_MODIFIED -> 503, retryable) from resolution failures + // (ENTITY_CANNOT_BE_RESOLVED / CATALOG_PATH_CANNOT_BE_RESOLVED -> 404), rather than failing + // with an opaque 500. + PolarisMetaStoreManager spyMetaStore = spy(metaStoreManager); + final IcebergCatalog catalog = newIcebergCatalog(CATALOG_NAME, spyMetaStore); + catalog.initialize( + CATALOG_NAME, + ImmutableMap.of( + CatalogProperties.FILE_IO_IMPL, "org.apache.iceberg.inmemory.InMemoryFileIO")); + + Namespace namespace = Namespace.of("rename_conflict_ns"); + catalog.createNamespace(namespace); + + final TableIdentifier from = TableIdentifier.of(namespace, "rename_from"); + final TableIdentifier to = TableIdentifier.of(namespace, "rename_to"); + catalog.buildTable(from, SCHEMA).create(); + + doReturn(new EntityResult(renameStatus, null)) + .when(spyMetaStore) + .renameEntity(any(), any(), any(), any(), any()); + + Assertions.assertThatThrownBy(() -> catalog.renameTable(from, to)) + .isInstanceOf(expectedException) + .hasMessageContaining("rename_from"); + } + @Test public void createCatalogWithReservedProperty() { Assertions.assertThatCode(