You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "tmater (via GitHub)" <gi...@apache.org> on 2025/07/28 17:33:42 UTC
[PR] Replace CommitFailedException with CommitConflictException [polaris]
tmater opened a new pull request, #2198:
URL: https://github.com/apache/polaris/pull/2198
In some cases, we were using CommitFailedException to represent commit conflicts, which returns the correct 409 response but is tied to Iceberg.
However, some of these conflicts originate from Polaris, making CommitConflictException a more appropriate and accurate choice.
This change updates those instances to improve clarity and exception handling semantics.
Resolves #2168
<!--
Possible security vulnerabilities: STOP here and contact security@apache.org instead!
Please update the title of the PR with a meaningful message - do not leave it "empty" or "generated"
Please update this summary field:
The summary should cover these topics, if applicable:
* the motivation for the change
* a description of the status quo, for example the current behavior
* the desired behavior
* etc
PR checklist:
- Do a self-review of your code before opening a pull request
- Make sure that there's good test coverage for the changes included in this PR
- Run tests locally before pushing a PR (./gradlew check)
- Code should have comments where applicable. Particularly hard-to-understand
areas deserve good in-line documentation.
- Include changes and enhancements to the documentation (in site/content/in-dev/unreleased)
- For Work In Progress Pull Requests, please use the Draft PR feature.
Make sure to add the information BELOW this comment.
Everything in this comment will NOT be added to the PR description.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace CommitFailedException with CommitConflictException [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2198:
URL: https://github.com/apache/polaris/pull/2198#discussion_r2237923902
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -997,7 +997,7 @@ public void commitTransaction(CommitTransactionRequest commitTransactionRequest)
callContext.getPolarisCallContext(), pendingUpdates);
if (!result.isSuccess()) {
// TODO: Retries and server-side cleanup on failure
- throw new CommitFailedException(
+ throw new CommitConflictException(
"Transaction commit failed with status: %s, extraInfo: %s",
result.getReturnStatus(), result.getExtraInformation());
Review Comment:
Strictly speaking the majority of possible status codes do not fit 409 responses, from my POV. It might be preferable to handle this case by inspecting `result.getReturnStatus()` (probably in the exception mapper).
That said, such a change will break current de-facto API contracts.... I'm not sure whether there is appetite in the community for clarifying this.
##########
spec/generated/bundled-polaris-catalog-service.yaml:
##########
@@ -635,7 +635,7 @@ paths:
TableToUpdateDoesNotExist:
$ref: '#/components/examples/NoSuchTableError'
'409':
- description: Conflict - CommitFailedException, one or more requirements failed. The client may retry.
+ description: Conflict - CommitConflictException, one or more requirements failed. The client may retry.
Review Comment:
I'd prefer to avoid mentioning rather internal Polaris exception type names in the API doc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace CommitFailedException with CommitConflictException [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #2198:
URL: https://github.com/apache/polaris/pull/2198#discussion_r2243355380
##########
spec/generated/bundled-polaris-catalog-service.yaml:
##########
@@ -635,7 +635,7 @@ paths:
TableToUpdateDoesNotExist:
$ref: '#/components/examples/NoSuchTableError'
'409':
- description: Conflict - CommitFailedException, one or more requirements failed. The client may retry.
+ description: Conflict - Transaction commit failed due to concurrent modifications. The client may retry.
Review Comment:
This file is generated; the change should go into the appropriate spec and then this file needs to be regenerated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace CommitFailedException with CommitConflictException [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2198:
URL: https://github.com/apache/polaris/pull/2198#discussion_r2243978003
##########
spec/generated/bundled-polaris-catalog-service.yaml:
##########
@@ -635,7 +635,7 @@ paths:
TableToUpdateDoesNotExist:
$ref: '#/components/examples/NoSuchTableError'
'409':
- description: Conflict - CommitFailedException, one or more requirements failed. The client may retry.
+ description: Conflict - Transaction commit failed due to concurrent modifications. The client may retry.
Review Comment:
Good point about spec change.
However, the code fix in this PR does not require spec changes. I believe the client-facing behaviour is not changed by this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace CommitFailedException with CommitConflictException [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2198:
URL: https://github.com/apache/polaris/pull/2198#discussion_r2243978839
##########
spec/generated/bundled-polaris-catalog-service.yaml:
##########
@@ -635,7 +635,7 @@ paths:
TableToUpdateDoesNotExist:
$ref: '#/components/examples/NoSuchTableError'
'409':
- description: Conflict - CommitFailedException, one or more requirements failed. The client may retry.
+ description: Conflict - Transaction commit failed due to concurrent modifications. The client may retry.
Review Comment:
@tmater : please rollback changes to this file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace CommitFailedException with CommitConflictException [polaris]
Posted by "tmater (via GitHub)" <gi...@apache.org>.
tmater commented on code in PR #2198:
URL: https://github.com/apache/polaris/pull/2198#discussion_r2240547488
##########
spec/generated/bundled-polaris-catalog-service.yaml:
##########
@@ -635,7 +635,7 @@ paths:
TableToUpdateDoesNotExist:
$ref: '#/components/examples/NoSuchTableError'
'409':
- description: Conflict - CommitFailedException, one or more requirements failed. The client may retry.
+ description: Conflict - CommitConflictException, one or more requirements failed. The client may retry.
Review Comment:
Good point, removed the exception and rephrased the description a bit.
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -997,7 +997,7 @@ public void commitTransaction(CommitTransactionRequest commitTransactionRequest)
callContext.getPolarisCallContext(), pendingUpdates);
if (!result.isSuccess()) {
// TODO: Retries and server-side cleanup on failure
- throw new CommitFailedException(
+ throw new CommitConflictException(
"Transaction commit failed with status: %s, extraInfo: %s",
result.getReturnStatus(), result.getExtraInformation());
Review Comment:
Yeah, I agree — I reverted the change and added a note to also keep an eye on the exceptions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace CommitFailedException with CommitConflictException [polaris]
Posted by "tmater (via GitHub)" <gi...@apache.org>.
tmater commented on code in PR #2198:
URL: https://github.com/apache/polaris/pull/2198#discussion_r2245046473
##########
spec/generated/bundled-polaris-catalog-service.yaml:
##########
@@ -635,7 +635,7 @@ paths:
TableToUpdateDoesNotExist:
$ref: '#/components/examples/NoSuchTableError'
'409':
- description: Conflict - CommitFailedException, one or more requirements failed. The client may retry.
+ description: Conflict - Transaction commit failed due to concurrent modifications. The client may retry.
Review Comment:
Makes sense, reverted it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace CommitFailedException with CommitConflictException [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b merged PR #2198:
URL: https://github.com/apache/polaris/pull/2198
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org