adutra commented on code in PR #1989:
URL: https://github.com/apache/polaris/pull/1989#discussion_r2224990697
##########
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);
Review Comment:
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`:
https://github.com/apache/polaris/blob/de351deb5648d4785639b0150d0c9df2aaab5b98/service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java#L927-L937
* Conflict when updating a `Principal`:
https://github.com/apache/polaris/blob/de351deb5648d4785639b0150d0c9df2aaab5b98/service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java#L1064-L1074
And so on for `PrincipalRole` and `CatalogRole`.
Besides, `CommitFailedException` is already being mapped to 409:
https://github.com/apache/polaris/blob/a303a1d70df3733f59914bcff62d3fe1a0fc3edc/service/common/src/main/java/org/apache/polaris/service/exception/IcebergExceptionMapper.java#L177
In short, I don't mind introducing this new type, but unless I'm missing
something obvious, I see little value in 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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
