Make location overlap checks more robust by dimas-b · Pull Request #4408 · apache/polaris

dimas-b · 2026-05-12T02:10:57Z

Feed sibling paths to the Resolver as optional and ignore non-found cases.

Note: it is normal for an entity to be deleted during the sibling resolution process. Ignoring unresolved entities is to compensate for data changes between the "list" operation and the "resolveAll" operation.

Throw a CommitConflictException if coherent data about all siblings cannot be resolved.

Subsequently, sibling resolution failures lead to 409 (Conflict) errors at the REST API level instead of 500 (Server Failure) now.

Fixes #4407

XN137 · 2026-05-12T05:43:53Z

+   * if this exceptio escaped, instead of Server Error (500).
+   */
+  @VisibleForTesting
+  static class UnableToCheckSiblingLocationsException extends CommitFailedException {


CommitFailedException javadoc reads:

Exception raised when a commit fails because of out of date metadata

do i assume correctly that we want to trigger retry behavior on the client side for this case and this makes it the best choice still?

not really, reties were server-side, but I refactored that part completely.

CommitFailedException was used only for the 409 error code.

snazy

I think this is better than the current behavior by returning 409 instead of 500.

That said, I am not convinced the retry loop is the right fix for the underlying problem.

The fallback path currently does two things:

list sibling names
resolve those siblings again by path

The concurrent delete race (your Concurrent sibling removal now causes a retry instead of hard failure point) seems to come from step 2. A sibling can show up in the list result and then disappear before resolveAll() runs, so the code creates a transient inconsistency for itself and then retries around it.

Because of that, this feels more like a mitigation than a real fix. It reduces the failure probability, but it also adds repeated work with a short retry delay (10 ms) on a hot path and still leaves an eventual failure mode under load once the timeout is hit.

I think the cleaner approach would be to avoid the second resolution step and load sibling entities directly once we already have the sibling ids / names, for example with listFullEntitiesAll(...) or listEntities(...) plus loadEntity(...) by id. With that, a concurrently removed sibling is just gone / skippable, instead of a loop that is prone to hit resolver failures. Similar to what PolicyCatalog.listPolicies() does.

Also, using CommitFailedException mainly as a transport to get a 409 feels a bit off semantically. A Polaris-specific exception that clearly maps to 409 seems better.

dimas-b · 2026-05-12T13:40:11Z

[...] the cleaner approach would be to avoid the second resolution step and load sibling entities directly once we already have the sibling ids / names

I was thinking about that too, but it would be a major change to the current Resolver logic and might alter expectations other Persistence implementations have on the end-to-end check/save process.

However, since @snazy prefers this approach too, I'm going to make this change.

I'm going to keep current commits in the PR for reference.

CC: @dennishuo

dimas-b · 2026-05-12T23:49:01Z

I refactored for a composite approach: lookup is optional now, but still goes through Resolver for maximum backward compatibility.

flyrain · 2026-05-14T01:13:17Z

+    for (PolarisEntity entityToCheck :
+        resolveOptionalPaths(pathsToResolve, parentPath.getFirst().getName())) {
+      String loc =
+          entityToCheck.getPropertiesAsMap().get(PolarisEntityConstants.ENTITY_BASE_LOCATION);


Regression for generic tables: GenericTableEntity.getBaseLocation() reads from getInternalPropertiesAsMap(), not getPropertiesAsMap(). So this lookup returns null for any generic-table sibling, the continue below skips it, and overlap is silently no longer enforced for generic tables. The pre-PR code had a subType == GENERIC_TABLE branch that handled this. I'd suggest to add a unit test to avoid future regression.

Good catch, @flyrain !

What's the rationale for this difference in Generic Tables?

I remember there are some discussion before, but couldn't remember the specific reason. cc @gh-yzou

flyrain · 2026-05-14T01:13:17Z

+
+      if (targetLocation.isChildOf(siblingLocation) || siblingLocation.isChildOf(targetLocation)) {
+        throw new ForbiddenException(
+            "Unable to create table at location '%s' because it conflicts with existing table or namespace at "


This method also runs for namespaces, message reads weird in that case. "Unable to create entity" or similar.

The message is the same as before. I can adjust it if you prefer.

That'd be awesome! It's not a blocker for me.

flyrain · 2026-05-14T01:13:17Z

+      if (status.getStatus().equals(ResolverStatus.StatusEnum.PATH_COULD_NOT_BE_FULLY_RESOLVED)) {
+        ResolverPath path = status.getFailedToResolvePath();
+        if (path != null) {
+          message += ". path: " + String.join(".", path.entityNames());


Minor: two sequential if (status.getStatus().equals(...)) on the same enum, switch or else if reads cleaner.

flyrain · 2026-05-14T01:22:30Z

+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+class IcebergCatalogTest {


Can we rename it to something specific like IcebergCatalogSiblingResolutionTest. The current name IcebergCatalogTest reads like the canonical test for the whole class and collides with the AbstractIcebergCatalogTest hierarchy.

IcebergCatalogTest is a unit test for (some of) the code inside IcebergCatalog. Other test cases may be added later for other parts of the code. I think the name matches the usual JUnit naming conventions.

flyrain

Thanks @dimas-b for the PR. Nice cleanup overall. The optional paths + 409-via-CommitConflictException is the right move and aligns with how the resolver wiki documents partial-resolution tolerance.

We may add an IT (or AbstractIcebergCatalogTest case) that deletes a sibling between list and resolveAll and asserts no 500, that's the actual race fixed by #4407.

dimas-b · 2026-05-14T18:04:01Z

@flyrain : an IT for those 500 errors is possible, but I do not think it's worth the effort and extra resource use during CI. Because of the racy nature of the issue, such a test cannot produce true positive results.

* Add time-limited retries to `validateNoLocationOverlap` * Add `SIBLING_TIMEOUT_MILLIS` to `FeatureConfiguration` to allow users to control the total retry duration. * Throw a `CommitFailedException` subclass if coherent data about all siblings cannot be resolved during the retry period. * Concurrent sibling removal now causes a retry instead of hard failure. * Final sibling resolution failures lead to 409 (Conflict) errors at the REST API level instead of 500 (Server Failure) now. * Sibling resolution can still fail under massive concurrent load of DELETE requests, but the probability of that should be reduced now and users can control it via the timeout parameter. Closes apache#4407

Feed sibling paths to the `Resolver` as optional and ignore non-found cases. Note: it is normal for an entity to be deleted during the sibling resolution process. Ignoring unresolved entities is to compensate for data changes between the "list" operation and the "resolveAll" operation. Throw a `CommitConflictException` if coherent data about all siblings cannot be resolved. Subsequently, sibling resolution failures lead to 409 (Conflict) errors at the REST API level instead of 500 (Server Failure) now. Fixes apache#4407

github-project-automation Bot added this to Basic Kanban Board May 12, 2026

github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board May 12, 2026

dimas-b requested review from XN137, collado-mike and snazy May 12, 2026 02:11

XN137 reviewed May 12, 2026

View reviewed changes

snazy reviewed May 12, 2026

View reviewed changes

dimas-b marked this pull request as draft May 12, 2026 13:40

dimas-b force-pushed the loc-checks-fix branch 4 times, most recently from c46d3a4 to b87cc5a Compare May 12, 2026 23:47

dimas-b marked this pull request as ready for review May 12, 2026 23:47

dimas-b requested review from XN137 and snazy May 12, 2026 23:48

flyrain reviewed May 14, 2026

View reviewed changes

dimas-b added 5 commits May 14, 2026 14:08

review: support generic tables

0d40d4c

review: adjust exception message

682fccb

review: else if

bdb1181

dimas-b force-pushed the loc-checks-fix branch from b87cc5a to bdb1181 Compare May 14, 2026 21:51

dimas-b requested a review from flyrain May 14, 2026 21:52

dimas-b added 2 commits May 14, 2026 19:39

fix PolarisS3InteroperabilityTest

8fa0d4d

support delegator for PolarisCatalogGenericTableApiService in tests

8ee2e48

Conversation

dimas-b commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

dimas-b commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimas-b commented May 12, 2026

Uh oh!

flyrain May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

dimas-b commented May 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dimas-b commented May 12, 2026 •

edited

Loading

dimas-b May 12, 2026 •

edited

Loading

dimas-b commented May 12, 2026 •

edited

Loading

flyrain May 14, 2026 •

edited

Loading

flyrain May 14, 2026 •

edited

Loading

flyrain May 14, 2026 •

edited

Loading