Respond with 409 in case of concurrent Namespace update failures instead of 500 #1989

fabio-rizzo-01 · 2025-07-02T10:28:25Z

I followed the same pattern used in methods of the same class, like commit(...). This will return a 409 back to the caller instead of a 500 error

Solves (#1390)

…pache#1390

singhpk234

Thank you for the change @fabio-rizzo-01, can you please add UTs for the same ?

…pache#1390: updated unit tests

fabio-rizzo-01 · 2025-07-02T15:22:07Z

Thank you for the change @fabio-rizzo-01, can you please add UTs for the same ?

Test added

singhpk234

Mostly LGTM ! Have one minor suggestion

cc @dimas-b as we discussed this before !

singhpk234 · 2025-07-02T15:23:55Z

...ime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java

+        .updateEntityPropertiesIfNotChanged(any(), any(), any());
+    assertThrows(
+        CommitFailedException.class, () -> catalog.setProperties(NS, ImmutableMap.of("a", "b")));
+    assertThrows(CommitFailedException.class, () -> catalog.removeProperties(NS, Set.of("a", "b")));


CommitFailedException would throw 409 from IcebergException mapper, but is there a way to just test the status code without relying on exception mapper on this case ?

That code is in the Exception mapper, I can add that specific case in the test:

polaris/service/common/src/test/java/org/apache/polaris/service/exception/IcebergExceptionMapperTest.java

Line 73 in eb6b6ad

Arguments.of(new RuntimeException("Error persisting entity"), 500)),

like this
Arguments.of(new CommitFailedException("Error persisting entity"), 409)),

but I think that at the catalog level we have to check for the exception.

singhpk234 · 2025-07-02T15:25:16Z

service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java

@@ -756,7 +756,7 @@ public boolean removeProperties(Namespace namespace, Set<String> properties)
            .map(PolarisEntity::new)
            .orElse(null);
    if (returnedEntity == null) {
-      throw new RuntimeException("Concurrent modification of namespace: " + namespace);
+      throw new CommitFailedException("Concurrent modification of namespace: %s", namespace);


I am fine with overloading the CommitFailedException as the iceberg exception for that doesn't say specifically anything about the entities for which commit failed.

IMHO, CommitFailedException is too generic to be used for reporting conflicts.

I'd prefer to have a Polaris-specific exception class specifically for reporting conflicts and mapping them to 409 at the REST API layer.

Ok, so should I add a new exception in polaris core and add the mapping in PolarisExceptionMapper? Any suggestion for the exception name is welcome.

I think having a polaris-specific exception for this case will simplify maintenance and allow fine-tuning response... so please so.

As for the name, I'd propose org.apache.polaris.core.exceptions.CommitConflictException extending PolarisException.

Update: please see my other comment.

Sorry, I was not really up-to-date on current code when I commented earlier.

Sorry for chiming in late, and also apologies if I'm missing some context here.

I am not convinced that we should introduce CommitConflictException. I see many usages of CommitFailedException in similar situations, for example:

Conflict when updating a Catalog:

polaris/service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java

Lines 927 to 937 in de351de

CatalogEntity returnedEntity =

Optional.ofNullable(

CatalogEntity.of(

PolarisEntity.of(

metaStoreManager.updateEntityPropertiesIfNotChanged(

getCurrentPolarisContext(), null, updatedEntity))))

.orElseThrow(

() ->

new CommitFailedException(

"Concurrent modification on Catalog '%s'; retry later", name));

return returnedEntity;

Conflict when updating a Principal:

polaris/service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java

Lines 1064 to 1074 in de351de

PrincipalEntity returnedEntity =

Optional.ofNullable(

PrincipalEntity.of(

PolarisEntity.of(

metaStoreManager.updateEntityPropertiesIfNotChanged(

getCurrentPolarisContext(), null, updatedEntity))))

.orElseThrow(

() ->

new CommitFailedException(

"Concurrent modification on Principal '%s'; retry later", name));

return returnedEntity;

And so on for PrincipalRole and CatalogRole.

Besides, CommitFailedException is already being mapped to 409:

polaris/service/common/src/main/java/org/apache/polaris/service/exception/IcebergExceptionMapper.java

Line 177 in a303a1d

case CommitFailedException e -> Status.CONFLICT.getStatusCode();

In short, I don't mind introducing this new type, but unless I'm missing something obvious, I see little value in it.

My point against CommitFailedException is that it comes from Iceberg API jar and as such is not related to the Iceberg REST API directly. Moreover, Polaris needs to have flexibility in producing REST responses, which is not provided for by CommitFailedException in general.

I'd personally prefer to gradually migrate all REST error handling to Polaris exceptions in cases where the error is triggered by Polaris code.

I think it's fine to have a mix of exceptions during the transition period.

Ok then... let's not forget to do the migration later, it would be nice to have a consistent handling of this kind of conflicts in Polaris.

dimas-b · 2025-07-03T15:49:28Z

service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java

@@ -756,7 +756,7 @@ public boolean removeProperties(Namespace namespace, Set<String> properties)
            .map(PolarisEntity::new)
            .orElse(null);
    if (returnedEntity == null) {
-      throw new RuntimeException("Concurrent modification of namespace: " + namespace);


I think returning 409 in this case is preferable to 500.

However, as far as #1390 is concerned, I believe Polaris can handle this case better. The Iceberg REST API defines property changes in terms or "removals" and "updates". Nothing is specified about checks for the previous state of Namespace properties. In that regard a read-then-write-if-not-modified pattern in the current Polaris code is prone to errors even if the property changes are non-conflicting in themselves.

I believe a better fix would be to add retries in case the entity is modified by another process in this middle of this operation.

Correction: I do not think a 409 response is justified in this case from the client's perspective. Per #1390 clients' requests are not conflicting with each other.

I can add the retry logic but there could still be a case when everything fails, so what do we want to return in that case?

I'd use a configurable retry timeout and the 408 status code when the timeout expires.

Actually 408 has a different purpose... Maybe 504 (Gateway Timeout) fits this use case.

@fabio-rizzo-01 : Could you start a dev email discussion on this for visibility?

@dimas-b email sent

…pache#1390

…apache#1390 # Conflicts: # service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java

dimas-b · 2025-07-22T15:10:51Z

service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java

-    }
+    return switch (exception) {
+      case AlreadyExistsException alreadyExistsException -> Response.Status.CONFLICT;
+      case CommitConflictException commitConflictException -> Response.Status.REQUEST_TIMEOUT;


I believe 408 does not match this use case. Per PRC 9110: The 408 (Request Timeout) status code indicates that the server did not receive a complete request message within the time that it was prepared to wait.

I think with the custom exception we can return 500 at this point, what do you think?

With the current state of the PR (no retries on persistence-level conflicts) I believe the 409 (Conflict) response code is the most relevant.

However, as discussed on the dev email thread, I do not think 409 is the best possible implementation of the Iceberg REST API. I believe Polaris should retry on the server side and respond with 429 (Too Many Requests) if retries do not help.

That said, I understand that implementing retries is a much bigger effort, I'd be ok with 409 for now (as it is incrementally better than 500).

ok so for now I'll use 409 and we can have an enhancement for the retries, ok?

dimas-b

Thanks, @fabio-rizzo-01 ! I'll leave this PR over for a couple of days for other people to review.

fabio-rizzo-01 · 2025-07-24T16:25:50Z

good to be merged? I'll create a new task for the retry logic @dimas-b

Concurrent Namespace Update Fails with RuntimeException and 500 Error a…

c0b57dc

…pache#1390

github-project-automation bot added this to Basic Kanban Board Jul 2, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Jul 2, 2025

singhpk234 reviewed Jul 2, 2025

View reviewed changes

fabio-rizzo-01 added 2 commits July 2, 2025 16:14

Concurrent Namespace Update Fails with RuntimeException and 500 Error a…

78b18e4

…pache#1390: updated unit tests

Concurrent Namespace Update Fails with RuntimeException and 500 Error a…

fd6b848

…pache#1390: updated unit tests

singhpk234 reviewed Jul 2, 2025

View reviewed changes

singhpk234 requested a review from dimas-b July 2, 2025 15:26

dimas-b reviewed Jul 3, 2025

View reviewed changes

fabio-rizzo-01 added 3 commits July 18, 2025 09:23

Concurrent Namespace Update Fails with RuntimeException and 500 Error a…

d3e2859

…pache#1390

Concurrent Namespace Update Fails with RuntimeException and 500 Error a…

d8ce2a7

…pache#1390

Merge remote-tracking branch 'origin/feature/apache#1390' into feature/…

7b93681

…apache#1390 # Conflicts: # service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java

fabio-rizzo-01 force-pushed the feature/#1390 branch from 4959c60 to 7b93681 Compare July 22, 2025 12:51

removed no needed test

aa8a9b9

dimas-b reviewed Jul 22, 2025

View reviewed changes

updated response code

9e0bd3c

dimas-b previously approved these changes Jul 22, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jul 22, 2025

dimas-b changed the title ~~Concurrent Namespace Update Fails with RuntimeException and 500 Error…~~ Respond with 409 in case of concurrent Namespace update failures instead of 500 Jul 22, 2025

eric-maynard previously approved these changes Jul 22, 2025

View reviewed changes

Merge branch 'main' into feature/apache#1390

365bf4e

fabio-rizzo-01 dismissed stale reviews from eric-maynard and dimas-b via 365bf4e July 23, 2025 16:07

dimas-b approved these changes Jul 23, 2025

View reviewed changes

dimas-b mentioned this pull request Jul 23, 2025

Consolidate usage of CommitFailedException and CommitConflictException #2168

Closed

dimas-b merged commit e9267b6 into apache:main Jul 24, 2025
12 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Jul 24, 2025

fabio-rizzo-01 mentioned this pull request Jul 25, 2025

Add retry logic to handle concurrent updates for namespace properties #2183

Open

	CatalogEntity returnedEntity =
	Optional.ofNullable(
	CatalogEntity.of(
	PolarisEntity.of(
	metaStoreManager.updateEntityPropertiesIfNotChanged(
	getCurrentPolarisContext(), null, updatedEntity))))
	.orElseThrow(
	() ->
	new CommitFailedException(
	"Concurrent modification on Catalog '%s'; retry later", name));
	return returnedEntity;

	PrincipalEntity returnedEntity =
	Optional.ofNullable(
	PrincipalEntity.of(
	PolarisEntity.of(
	metaStoreManager.updateEntityPropertiesIfNotChanged(
	getCurrentPolarisContext(), null, updatedEntity))))
	.orElseThrow(
	() ->
	new CommitFailedException(
	"Concurrent modification on Principal '%s'; retry later", name));
	return returnedEntity;

Respond with 409 in case of concurrent Namespace update failures instead of 500 #1989

Respond with 409 in case of concurrent Namespace update failures instead of 500 #1989

Uh oh!

Conversation

fabio-rizzo-01 commented Jul 2, 2025 • edited by singhpk234 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

singhpk234 left a comment

Choose a reason for hiding this comment

Uh oh!

fabio-rizzo-01 commented Jul 2, 2025

Uh oh!

singhpk234 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabio-rizzo-01 Jul 2, 2025 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Jul 3, 2025 • 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!

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!

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!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

fabio-rizzo-01 commented Jul 24, 2025

Uh oh!

Uh oh!

Uh oh!

fabio-rizzo-01 commented Jul 2, 2025 •

edited by singhpk234

Loading

fabio-rizzo-01 Jul 2, 2025 •

edited

Loading

dimas-b Jul 3, 2025 •

edited

Loading