Clean up orphan metadata files on failed table/view commits#5057
Conversation
There was a problem hiding this comment.
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
doCommitpersistence logic intry/finallyto 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
registerTabledoes 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. |
| PolarisStorageActions.LIST)); | ||
|
|
||
| String newLocation = writeNewMetadataIfRequired(base == null, metadata); | ||
| boolean metadataWritten = !(base == null && metadata.metadataFileLocation() != null); |
There was a problem hiding this comment.
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>?
doCommitfortablesandviewswrites 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
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)