amogh-jahagirdar commented on code in PR #5459:
URL: https://github.com/apache/iceberg/pull/5459#discussion_r956596984


##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -151,27 +132,69 @@ private static void deleteFiles(FileIO io, 
Set<ManifestFile> allManifests) {
         .run(
             manifest -> {
               try (ManifestReader<?> reader = ManifestFiles.open(manifest, 
io)) {
+                List<String> pathsToDelete = Lists.newArrayList();
                 for (ManifestEntry<?> entry : reader.entries()) {
                   // intern the file path because the weak key map uses 
identity (==) instead of
                   // equals
                   String path = entry.file().path().toString().intern();
                   Boolean alreadyDeleted = deletedFiles.putIfAbsent(path, 
true);
                   if (alreadyDeleted == null || !alreadyDeleted) {
-                    try {
-                      io.deleteFile(path);
-                    } catch (RuntimeException e) {
-                      // this may happen if the map of deleted files gets 
cleaned up by gc
-                      LOG.warn("Delete failed for data file: {}", path, e);
-                    }
+                    pathsToDelete.add(path);
                   }
                 }
+
+                String type = reader.isDeleteManifestReader() ? "delete" : 
"data";
+                deleteFiles(io, pathsToDelete, type, false);
               } catch (IOException e) {
                 throw new RuntimeIOException(
                     e, "Failed to read manifest file: %s", manifest.path());
               }
             });
   }
 
+  /**
+   * Helper to delete files. Bulk deletion is used if possible.
+   *
+   * @param io FileIO for deletes
+   * @param files files to delete
+   * @param type type of files being deleted
+   * @param concurrent controls concurrent deletion. Only applicable for 
non-bulk FileIO
+   */
+  private static void deleteFiles(
+      FileIO io, Iterable<String> files, String type, boolean concurrent) {
+    if (io instanceof SupportsBulkOperations) {
+      try {
+        SupportsBulkOperations bulkIo = (SupportsBulkOperations) io;

Review Comment:
   Good catch, fixed!



-- 
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]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to