[GitHub] [flink] reswqa commented on a diff in pull request #21673: [FLINK-30513] Cleanup HA storage path on cluster termination

["reswqa (via GitHub)" <gi...@apache.org>]

reswqa commented on code in PR #21673:
URL: https://github.com/apache/flink/pull/21673#discussion_r1121208835


##########
flink-runtime/src/main/java/org/apache/flink/runtime/highavailability/AbstractHaServices.java:
##########
@@ -225,6 +237,13 @@ public CompletableFuture<Void> globalCleanupAsync(JobID jobID, Executor executor
                 executor);
     }
 
+    protected void cleanupClusterHaStoragePath() throws Exception {
+        final Path clusterHaStoragePath =
+                HighAvailabilityServicesUtils.getClusterHighAvailableStoragePath(configuration);
+        final FileSystem fileSystem = clusterHaStoragePath.getFileSystem();
+        fileSystem.delete(clusterHaStoragePath, true);

Review Comment:
   Is this actually already delete the blob dir here? Because this will delete the parent path recursively. But the protocol of `BlobStoreService#closeAndCleanupAllData` maybe not limited to deleting directory, just the current implementation is. Maybe the call of `blobStoreService.closeAndCleanupAllData()` before remove the parent dir just looks a little confused but still needs to be kept.



##########
flink-runtime/src/test/java/org/apache/flink/runtime/highavailability/AbstractHaServicesTest.java:
##########
@@ -44,18 +44,20 @@
 
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;

Review Comment:
   We should avoid use hamcrest now. Maybe you can migrate this test class to junit5 and assertj first.



-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org