dimas-b commented on code in PR #1989:
URL: https://github.com/apache/polaris/pull/1989#discussion_r2183138817
##########
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);
Review Comment:
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.
--
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
