Skip to content

Core: Handle bulk deletion exceptions in CatalogUtil#15464

Open
sumtiogo wants to merge 1 commit intoapache:mainfrom
sumtiogo:fix-catalogutil-bulk-delete
Open

Core: Handle bulk deletion exceptions in CatalogUtil#15464
sumtiogo wants to merge 1 commit intoapache:mainfrom
sumtiogo:fix-catalogutil-bulk-delete

Conversation

@sumtiogo
Copy link

@sumtiogo sumtiogo commented Feb 27, 2026

I recently opened Issue #15456 regarding this missing exception handling. Since the fix is relatively straightforward and I have already verified it with a unit test locally, I went ahead and created this PR to facilitate the discussion and review process.

Fixes #15456

Why are the changes needed?

In CatalogUtil.deleteRemovedMetadataFiles, there is an asymmetry in how exceptions are handled between bulk operations and single-file deletions.

When the FileIO supports bulk operations, deleteFiles is called directly without failure suppression. However, implementations of SupportsBulkOperations.deleteFiles can and do throw exceptions (e.g., BulkDeletionFailureException in S3FileIO when S3 DeleteObjects fails). Because this cleanup operation is intended to be best-effort, a system-level or network exception here should be caught and logged rather than bubbling up and potentially crashing the main calling process. The single-file deletion (else branch) already handles this correctly using Tasks.suppressFailureWhenFinished().

What changes were proposed in this pull request?

  • Wrapped the SupportsBulkOperations.deleteFiles call in a try-catch block within CatalogUtil.deleteRemovedMetadataFiles.
  • Added a warning log to record the failure and the number of files that failed to be deleted, maintaining consistency with how exceptions are handled in other bulk deletion cleanups (e.g., BaseTransaction).

How was this patch tested?

  • Added a new unit test testDeleteRemovedMetadataFilesBulkDeletionFailure in TestCatalogUtil using Mockito to simulate a RuntimeException during bulk deletion and verified that it is safely caught without throwing.
  • Ran ./gradlew spotlessApply and ./gradlew :iceberg-core:test locally.

@github-actions github-actions bot added the core label Feb 27, 2026
@sumtiogo sumtiogo force-pushed the fix-catalogutil-bulk-delete branch from 7fc52f2 to 62b5ff3 Compare February 27, 2026 12:42
@sumtiogo sumtiogo force-pushed the fix-catalogutil-bulk-delete branch from 62b5ff3 to 8ec13df Compare February 27, 2026 12:57

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatCode;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import static org.assertj.core.api.AssertionsForClassTypes.assertThatCode;
import static org.assertj.core.api.Assertions.assertThatCode;

.deleteFiles(
Iterables.transform(
removedPreviousMetadataFiles, TableMetadata.MetadataLogEntry::file));
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than adding this try-catch block, we should probably just call deleteFiles() here directly, which internally already catches exceptions.

deleteFiles(
          io,
          removedPreviousMetadataFiles.stream()
              .map(TableMetadata.MetadataLogEntry::file)
              .collect(Collectors.toSet()),
          "metadata",
          true)

with that you can also remove the parallel delete in the else block

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Core: Missing exception handling for bulk deletion in CatalogUtil.deleteRemovedMetadataFiles

2 participants