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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
