Skip to content

Clean up orphan metadata files on failed table/view commits#5057

Merged
dimas-b merged 1 commit into
apache:mainfrom
ayushtkn:cleanupCommit
Jul 16, 2026
Merged

Clean up orphan metadata files on failed table/view commits#5057
dimas-b merged 1 commit into
apache:mainfrom
ayushtkn:cleanupCommit

Conversation

@ayushtkn

Copy link
Copy Markdown
Member

doCommit for tables and views writes new metadata JSON to object storage before the metastore CAS check. When that check fails (concurrent writer, persistence error, etc.), the new file was left behind as an orphan — unreferenced by any snapshot and invisible to Iceberg's normal metadata cleanup.

The fix wraps the post-write commit logic in try/finally and deletes newLocation on failure, but only when this commit actually wrote the file (not for caller-supplied paths like registerTable).

Checklist

  • 🛡️ Don't disclose security issues! (contact security@apache.org)
  • 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
  • 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
  • 💡 Added comments for complex logic
  • 🧾 Updated CHANGELOG.md (if needed)
  • 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

Copilot AI review requested due to automatic review settings July 14, 2026 06:03
@github-project-automation github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Jul 14, 2026

Copilot AI 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.

Pull request overview

This PR addresses orphaned Iceberg metadata JSON files that could be left in object storage when LocalIcebergCatalog writes new table/view metadata before the metastore CAS step and that CAS step fails (e.g., due to concurrent modification). The change introduces best-effort cleanup of newly written metadata files on failed commits, while avoiding deletion for caller-supplied metadata paths (e.g., registerTable).

Changes:

  • Wrap table and view doCommit persistence logic in try/finally to delete newly written metadata files when the commit fails.
  • Reuse a shared cleanup helper (IcebergCatalogHandler.cleanupWrittenMetadataFiles) for deleting written-but-uncommitted metadata locations.
  • Add tests covering (1) failed table commit cleanup, (2) ensuring registerTable does not delete externally provided metadata, and (3) failed view commit cleanup.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java Adds try/finally commit cleanup for table/view metadata writes when commits fail.
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java Exposes (package-private) helper types/methods to perform best-effort cleanup of written metadata files.
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogTest.java Adds table commit failure test verifying orphan metadata is deleted; adds registerTable “do not delete caller file” test.
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogViewTest.java Adds view commit failure test verifying attempted orphan metadata cleanup via deleteFile.

@dimas-b dimas-b 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.

Good catch, @ayushtkn !

Changes LGTM with a minor comment 😉

PolarisStorageActions.LIST));

String newLocation = writeNewMetadataIfRequired(base == null, metadata);
boolean metadataWritten = !(base == null && metadata.metadataFileLocation() != null);

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.

Given that writeNewMetadataIfRequired is not private, this logic is very obscure 🤔 Could we use a Consumer<Boolean> parameter for collecting the "written" flag instead?.. or perhaps return a small record <boolean, String>?

dimas-b
dimas-b previously approved these changes Jul 16, 2026

@dimas-b dimas-b 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 update, @ayushtkn ! Changes LGTM 👍 but there are merge conflicts now 😅

@github-project-automation github-project-automation Bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jul 16, 2026
@dimas-b
dimas-b merged commit 4215a84 into apache:main Jul 16, 2026
25 checks passed
@github-project-automation github-project-automation Bot moved this from Ready to merge to Done in Basic Kanban Board Jul 16, 2026
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.

3 participants