Skip to content

Return HTTP 503 on concurrent rename instead of 500 #4646

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vigneshio wants to merge 1 commit into
base: main
Choose a base branch
from
+69 −3
Open

Return HTTP 503 on concurrent rename instead of 500 #4646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down
19 changes: 16 additions & 3 deletions runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -2494,10 +2495,22 @@ private void renameTableLike(
case BaseResult.ReturnStatus.ENTITY_NOT_FOUND:

@adutra adutra Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the goal is to tackle concurrent renames, I think we need to handle CATALOG_PATH_CANNOT_BE_RESOLVED as well. It's raised when a catalog path resolution fails during a write, see TransactionalMetaStoreManagerImpl.renameEntity(). I suggest it be mapped to NoSuchNamespaceException.

@dimas-b dimas-b Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM 👍

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:

@nandorKollar nandorKollar Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should narrow down this to case BaseResult.ReturnStatus.TARGET_ENTITY_CONCURRENTLY_MODIFIED, I'm not sure about ENTITY_CANNOT_BE_RESOLVED. Can a retry solve the problem with entity resolution?

@adutra adutra Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ENTITY_CANNOT_BE_RESOLVED is very similar to CATALOG_PATH_CANNOT_BE_RESOLVED: in TransactionalMetaStoreManagerImpl.renameEntity() the former is raised when the old path cannot be resolved, and the latter, when the new path cannot be resolved.

polaris/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java

Lines 1205 to 1207 in 005e889

if (resolver.isFailure()) {
return new EntityResult(BaseResult.ReturnStatus.ENTITY_CANNOT_BE_RESOLVED, null);
}

polaris/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java

Lines 1238 to 1240 in 005e889

if (resolver.isFailure()) {
return new EntityResult(BaseResult.ReturnStatus.CATALOG_PATH_CANNOT_BE_RESOLVED, null);
}

I'd note though that the NoSQL persistence does not raise CATALOG_PATH_CANNOT_BE_RESOLVED.

I'd suggest to group them together and throw NoSuchNamespaceException instead.

However, NoSuchNamespaceException maps to 404 which is non-retriable. But the comments on BaseResult for both codes say they are retriable:

polaris/polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/BaseResult.java

Lines 76 to 84 in 46e6891

// the specified catalog path cannot be resolved. There is a possibility that by the time a call
// is made by the client to the persistent storage, something has changed due to concurrent
// modification(s). The client should retry in that case.
CATALOG_PATH_CANNOT_BE_RESOLVED(3),
// the specified entity (and its path) cannot be resolved. There is a possibility that by the
// time a call is made by the client to the persistent storage, something has changed due to
// concurrent modification(s). The client should retry in that case.
ENTITY_CANNOT_BE_RESOLVED(4),

I actually think the comments are wrong. If either the old or new path has been deleted by a concurrent commit, clients should not retry.

@dimas-b dimas-b Jun 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting info, but IIRC TransactionalMetaStoreManagerImpl is not actually used in actual OSS call paths... perhaps only with the in-memory persistence, but JDBC does not use it either, I'm pretty sure 🤔

@adutra adutra Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ALWAYS get fooled by its name 😅

Looking at AtomicOperationMetaStoreManager.renameEntity() this time: oddly enough, it does not raise neither CATALOG_PATH_CANNOT_BE_RESOLVED nor ENTITY_CANNOT_BE_RESOLVED. It actually seems to not care about the validity of the entity path before and after 🤷‍♂️

@dimas-b dimas-b Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting... 🤔 @vigneshio : How did you hit these errors in practice?

@nandorKollar nandorKollar Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ENTITY_CANNOT_BE_RESOLVED is not a valid end expected response of a rename, then shouldn't we handle it as 'everything else', and throw new IllegalStateException( "Unknown error status " + returnedEntityResult.getReturnStatus());?

@adutra adutra Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ENTITY_CANNOT_BE_RESOLVED is used by the NoSQL persistence.

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:
Expand Down
52 changes: 52 additions & 0 deletions .../src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -2529,6 +2530,57 @@ public void testConcurrencyConflictUpdateTableDuringFinalTransaction() {
.hasMessageContaining("conflict_table");
}

static Stream<Arguments> 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<? extends Throwable> 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(
Expand Down