dimas-b commented on code in PR #4730:
URL: https://github.com/apache/polaris/pull/4730#discussion_r3417655437
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogViewIntegrationBase.java:
##########
@@ -124,6 +124,7 @@ public void before(TestInfo testInfo) {
.addProperty(
FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true")
.addProperty(FeatureConfiguration.DROP_WITH_PURGE_ENABLED.catalogConfig(),
"true")
+
.addProperty(FeatureConfiguration.PURGE_VIEW_METADATA_ON_DROP.catalogConfig(),
"false")
Review Comment:
Why is this necessary?
##########
runtime/service/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java:
##########
@@ -85,6 +86,11 @@ private Optional<IcebergTableLikeEntity>
tryGetTableEntity(TaskEntity task) {
@Override
public boolean handleTask(TaskEntity cleanupTask, CallContext callContext) {
IcebergTableLikeEntity tableEntity =
tryGetTableEntity(cleanupTask).orElseThrow();
+
+ if (tableEntity.getSubType() == PolarisEntitySubType.ICEBERG_VIEW) {
Review Comment:
This looks logically controversial 🤷 a "table" being confirmed to be a
"view" 🤔
Should we actually refactor `handleTask` to delegate to
`handleViewCleanup()` or to `handleTableCleanup()` based on sub-type?
##########
runtime/service/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java:
##########
@@ -151,6 +157,34 @@ public boolean handleTask(TaskEntity cleanupTask,
CallContext callContext) {
return false;
}
+ private boolean handleViewCleanup(TaskEntity cleanupTask,
IcebergTableLikeEntity viewEntity) {
+ LOGGER
+ .atInfo()
+ .addKeyValue("viewIdentifier", viewEntity.getTableIdentifier())
+ .addKeyValue("metadataLocation", viewEntity.getMetadataLocation())
+ .log("Handling view metadata cleanup task");
+
+ try (FileIO fileIO = fileIOSupplier.apply(cleanupTask,
viewEntity.getTableIdentifier())) {
+ if (!TaskUtils.exists(viewEntity.getMetadataLocation(), fileIO)) {
+ LOGGER
+ .atWarn()
+ .addKeyValue("viewIdentifier", viewEntity.getTableIdentifier())
+ .addKeyValue("metadataLocation", viewEntity.getMetadataLocation())
+ .log("View metadata cleanup scheduled, but metadata file does not
exist");
+ return true;
+ }
+
+ fileIO.deleteFile(viewEntity.getMetadataLocation());
Review Comment:
This seems to delete only the current metadata file, while the table purge
deletes "previous" metadata files too.
I'm a bit fuzzy on views, though... When a view is changed, do we get two
metadata files? one for the old version and new for the new?
If so, deleting only the latest file will probably leave a lot of older
metadata files behind... 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]
