dimas-b commented on code in PR #4013:
URL: https://github.com/apache/polaris/pull/4013#discussion_r2976914013
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java:
##########
@@ -2614,4 +2615,30 @@ public void testPaginatedListNamespaces() {
}
}
}
+
+ @Test
+ public void testUpdateNamespacePropertiesAtomic() {
Review Comment:
It might be preferable to promote this test to
`PolarisRestCatalogIntegrationBase`, which acts almost like a TCK for Polaris.
WDYT?
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java:
##########
@@ -2614,4 +2615,30 @@ public void testPaginatedListNamespaces() {
}
}
}
+
+ @Test
+ public void testUpdateNamespacePropertiesAtomic() {
Review Comment:
What in this test ensures that the operation is "atomic"? 🤔
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/CatalogHandlerUtils.java:
##########
@@ -239,13 +239,17 @@ public UpdateNamespacePropertiesResponse
updateNamespaceProperties(
Map<String, String> startProperties =
catalog.loadNamespaceMetadata(namespace);
Set<String> missing = Sets.difference(removals, startProperties.keySet());
- if (!updates.isEmpty()) {
- catalog.setProperties(namespace, updates);
- }
+ if (catalog instanceof IcebergCatalog polarisCatalog) {
Review Comment:
Yes, having `instanceof` checks in this case does not look nice from the
design POV.
If we have to have custom handling for the internal catalog (which is ok),
perhaps it is time to refactor this code to use proper polymorphic `catalog`
objects and allow implementations to decide how to best handle updates.
Federated catalogs will have to delegate to the appropriate API.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/CatalogHandlerUtils.java:
##########
@@ -239,13 +239,17 @@ public UpdateNamespacePropertiesResponse
updateNamespaceProperties(
Map<String, String> startProperties =
catalog.loadNamespaceMetadata(namespace);
Set<String> missing = Sets.difference(removals, startProperties.keySet());
- if (!updates.isEmpty()) {
- catalog.setProperties(namespace, updates);
Review Comment:
Does this mean Polaris propagates namespace property updates to Federated
catalogs even if `CatalogEntity.isStaticFacade()` is `true`? @dennishuo WDYT?
--
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]
