(amoro) branch master updated: Fix expireSnapshots fails with IllegalArgumentException on object-store-backed tables (#4259)

[jinsongzhou]

This is an automated email from the ASF dual-hosted git repository.

zhoujinsong pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/amoro.git


The following commit(s) were added to refs/heads/master by this push:
     new bc83f0834 Fix expireSnapshots fails with IllegalArgumentException on 
object-store-backed tables (#4259)
bc83f0834 is described below

commit bc83f0834d0f7c88d43f8add0312d29555aa063e
Author: felixhzhu <43262022+felixh...@users.noreply.github.com>
AuthorDate: Wed Jun 24 11:07:18 2026 +0800

    Fix expireSnapshots fails with IllegalArgumentException on 
object-store-backed tables (#4259)
    
    [AMORO-4237][Iceberg] Fix expireSnapshots fails with 
IllegalArgumentException on object-store-backed tables
    
    When the underlying FileIO is an object store 
(S3FileIO/OSSFileIO/GCSFileIO), deleteEmptyDirectory() calls asFileSystemIO() 
which throws IllegalArgumentException because supportFileSystemOperations() 
returns false.
    
    Fix: 1) Add early-return in TableFileUtil.deleteEmptyDirectory() when 
!supportFileSystemOperations() 2) Guard the parent directory cleanup loop in 
RollingFileCleaner with supportFileSystemOperations() check 3) Move buffer 
clear() into finally block to prevent re-submission on failure
    
    fix #4237
    
    Co-authored-by: felixhzhu <felixh...@tencent.com>
---
 .../formats/iceberg/utils/RollingFileCleaner.java  | 55 +++++++++++++---------
 .../java/org/apache/amoro/utils/TableFileUtil.java |  6 +++
 2 files changed, 39 insertions(+), 22 deletions(-)

diff --git 
a/amoro-format-iceberg/src/main/java/org/apache/amoro/formats/iceberg/utils/RollingFileCleaner.java
 
b/amoro-format-iceberg/src/main/java/org/apache/amoro/formats/iceberg/utils/RollingFileCleaner.java
index e453af95e..8d2e8ac06 100644
--- 
a/amoro-format-iceberg/src/main/java/org/apache/amoro/formats/iceberg/utils/RollingFileCleaner.java
+++ 
b/amoro-format-iceberg/src/main/java/org/apache/amoro/formats/iceberg/utils/RollingFileCleaner.java
@@ -92,32 +92,43 @@ public class RollingFileCleaner {
       return;
     }
 
-    if (fileIO.supportBulkOperations()) {
-      try {
-        fileIO.asBulkFileIO().deleteFiles(collectedFiles);
-        cleanedFileCounter.addAndGet(collectedFiles.size());
-      } catch (BulkDeletionFailureException e) {
-        LOG.warn("Failed to delete {} expired files in bulk", 
e.numberFailedObjects());
-      }
-    } else {
-      for (String filePath : collectedFiles) {
+    try {
+      if (fileIO.supportBulkOperations()) {
         try {
-          fileIO.deleteFile(filePath);
-          cleanedFileCounter.incrementAndGet();
-        } catch (Exception e) {
-          LOG.warn("Failed to delete expired file: {}", filePath, e);
+          fileIO.asBulkFileIO().deleteFiles(collectedFiles);
+          cleanedFileCounter.addAndGet(collectedFiles.size());
+        } catch (BulkDeletionFailureException e) {
+          LOG.warn("Failed to delete {} expired files in bulk", 
e.numberFailedObjects());
+        }
+      } else {
+        for (String filePath : collectedFiles) {
+          try {
+            fileIO.deleteFile(filePath);
+            cleanedFileCounter.incrementAndGet();
+          } catch (Exception e) {
+            LOG.warn("Failed to delete expired file: {}", filePath, e);
+          }
+        }
+      }
+      // Try to delete empty parent directories. Skipped automatically by
+      // TableFileUtil.deleteEmptyDirectory when the underlying FileIO is an 
object store.
+      if (fileIO.supportFileSystemOperations()) {
+        for (String parentDir : parentDirectories) {
+          try {
+            TableFileUtil.deleteEmptyDirectory(fileIO, parentDir, 
excludeFiles);
+          } catch (Exception e) {
+            LOG.warn("Failed to delete empty parent directory: {}", parentDir, 
e);
+          }
         }
       }
-    }
-    // Try to delete empty parent directories
-    for (String parentDir : parentDirectories) {
-      TableFileUtil.deleteEmptyDirectory(fileIO, parentDir, excludeFiles);
-    }
-    parentDirectories.clear();
-
-    LOG.debug("Cleaned expired a file group, total files: {}", 
collectedFiles.size());
 
-    collectedFiles.clear();
+      LOG.debug("Cleaned expired a file group, total files: {}", 
collectedFiles.size());
+    } finally {
+      // Always drop both buffers so a failure in directory cleanup does not 
cause the
+      // already-deleted files to be re-submitted on the next batch.
+      parentDirectories.clear();
+      collectedFiles.clear();
+    }
   }
 
   public int fileCount() {
diff --git 
a/amoro-format-iceberg/src/main/java/org/apache/amoro/utils/TableFileUtil.java 
b/amoro-format-iceberg/src/main/java/org/apache/amoro/utils/TableFileUtil.java
index 62d7ea651..1bbe17d01 100644
--- 
a/amoro-format-iceberg/src/main/java/org/apache/amoro/utils/TableFileUtil.java
+++ 
b/amoro-format-iceberg/src/main/java/org/apache/amoro/utils/TableFileUtil.java
@@ -87,6 +87,12 @@ public class TableFileUtil {
     if (directoryPath == null || directoryPath.isEmpty()) {
       return;
     }
+    // Object stores (S3FileIO / OSSFileIO / GCSFileIO ...) have no real 
directory concept.
+    // They are exposed via AuthenticatedFileIOAdapter whose 
supportFileSystemOperations() is
+    // false, so calling asFileSystemIO() would fail the precondition. Skip 
silently in that case.
+    if (!io.supportFileSystemOperations()) {
+      return;
+    }
     if (!io.exists(directoryPath)) {
       LOG.debug("The target directory {} does not exist or has been deleted", 
directoryPath);
       return;