adutra commented on code in PR #4646:
URL: https://github.com/apache/polaris/pull/4646#discussion_r3388312104
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2494,10 +2495,14 @@ private void renameTableLike(
case BaseResult.ReturnStatus.ENTITY_NOT_FOUND:
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
+ // Transient concurrency conditions: surface as 503 so clients can
retry. We avoid 409
+ // here 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:
case BaseResult.ReturnStatus.ENTITY_CANNOT_BE_RESOLVED:
Review Comment:
`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.
https://github.com/apache/polaris/blob/005e889983d71a51402944bb1c75d5baa549391f/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java#L1205-L1207
https://github.com/apache/polaris/blob/005e889983d71a51402944bb1c75d5baa549391f/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java#L1238-L1240
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:
https://github.com/apache/polaris/blob/46e6891a2c4f2f8b1d7e1015cff54d25a20a0df7/polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/BaseResult.java#L76-L84
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.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2494,10 +2495,14 @@ private void renameTableLike(
case BaseResult.ReturnStatus.ENTITY_NOT_FOUND:
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
+ // Transient concurrency conditions: surface as 503 so clients can
retry. We avoid 409
+ // here 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:
Review Comment:
I agree, and it's very unfortunate, that we can't use 409. It describes
_perfectly_ what happened, and it's retriable; but the Iceberg spec is very
opinionated and maps this code to the "already exists" case (and seems to imply
that the error is _not_ retriable, in contradiction with the HTTP spec).
We can't force a 409 for other use cases because clients would surface the
error as an `AlreadyExistsException`:
https://github.com/apache/iceberg/blob/17fc6da837442443421cfbac01ff2941a820ba20/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java#L156-L157
So, I agree that `ServiceUnavailableException` is the least worst choice.
It's retriable, which is all that counts.
Note: 429 (Too Many Requests) is also retriable but should in theory include
a `Retry-After` header in the response. It may trigger a client throttling that
would be undesirable.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2494,10 +2495,14 @@ private void renameTableLike(
case BaseResult.ReturnStatus.ENTITY_NOT_FOUND:
Review Comment:
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`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
