Skip to content

Preserve failed batch cleanup tasks for retry - #5421

Open
mattfaltyn wants to merge 2 commits into
apache:mainfrom
mattfaltyn:fix-batch-cleanup-error-propagation
Open

Preserve failed batch cleanup tasks for retry#5421
mattfaltyn wants to merge 2 commits into
apache:mainfrom
mattfaltyn:fix-batch-cleanup-error-propagation

Conversation

@mattfaltyn

@mattfaltyn mattfaltyn commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Fixes #5420.

Batch cleanup currently delegates to Iceberg's best-effort CatalogUtil.deleteFiles, which suppresses storage deletion errors. Polaris therefore skips its retries, reports success, and drops the persisted task while files remain undeleted.

This change propagates bulk deletion errors directly and uses error-propagating per-file deletion for other FileIO implementations. The existing batch loop keeps its three-attempt retry budget and backoff. Sequential/concurrent deletion is preserved, and an already-deleted file remains a successful outcome when retrying a partial batch.

The regression tests inject actual transient and permanent deletion failures instead of overriding retry recursion. They cover bulk and individual deletion, partial deletion in both execution modes, and task persistence/events followed by a successful replay. No public API, configuration, or persistence format changes are needed. Existing task retry/recovery scheduling is unchanged; this does not reconstruct tasks already lost before the fix.

Known limitation: Iceberg 1.11.0's ADLSFileIO.deleteFile suppresses runtime deletion failures before Polaris can observe them, so Azure cleanup is not covered by this fix. Tracked separately in #5482.

Validation completed on Ubuntu 24.04.4 / Java 21 using stock database images:

  • ./gradlew format compileAll :polaris-runtime-service:check passed. The module results contain 24,817 passes, 986 skips, and no failures or errors.
  • ./gradlew check passed: 28,515 passed, 1,011 skipped, and no failures or errors across 29,526 cases. Existing credential guards account for 873 runtime-cloud skips.
  • The 18 affected test cases all pass with the fix; 9 fail as expected against unmodified production code.
  • Existing Python client suites also passed: 199 unit and 15 integration cases.

Checklist

  • 🛡️ Don't disclose security issues! (contact security@apache.org)
  • 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes Batch file cleanup reports success and drops tasks after deletion failures #5420
  • 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
  • 💡 Added comments for complex logic
  • 🧾 Updated CHANGELOG.md (if needed)
  • 📚 Checked documentation in site/content/in-dev/unreleased; no changes needed because this restores cleanup retry behavior without changing APIs or configuration.

Prepared with significant AI assistance.

@github-project-automation github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Aug 31, 2026
@mattfaltyn
mattfaltyn marked this pull request as ready for review August 31, 2026 21:50

@vigneshio vigneshio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution @mattfaltyn.. LGTM. Resolve conflicts.

Propagate bulk and individual file deletion errors through the existing
batch retry loop instead of suppressing them. Keep partial deletion
idempotent and verify persisted tasks remain available for a later retry.

Fixes apache#5420
@mattfaltyn
mattfaltyn force-pushed the fix-batch-cleanup-error-propagation branch from ac75de4 to 2c71c63 Compare September 3, 2026 08:23
@mattfaltyn

Copy link
Copy Markdown
Contributor Author

Thanks for the contribution @mattfaltyn.. LGTM. Resolve conflicts.

Thanks @vigneshio! Rebased onto the latest main and resolved the conflict.

.run(
file -> {
try {
fileIO.deleteFile(file);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

IIUC, (non-blocking) this restores retries for failures reported by FileIO, but Azure still has a gap. Iceberg 1.11.0's ADLSFileIO.deleteFile catches RuntimeException and only logs it, so a failed deletion can still look successful and cause Polaris to drop the task. This predates the PR. Could we note the limitation and track the Azure case separately?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could we note the limitation and track the Azure case separately?

Absolutely—thanks for catching this. Documented in 250a360 and tracked in #5482.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Batch file cleanup reports success and drops tasks after deletion failures

3 participants