Repository navigation
feat(catalog): support write.metadata.delete-after-commit.enabled - #3582
Conversation
Add the `write.metadata.delete-after-commit.enabled` table property (default false). When it is set, catalogs that write metadata files themselves delete the previous metadata files that fall out of the metadata log (bounded by `write.metadata.previous-versions-max`) after a successful commit, matching Java's `CatalogUtil.deleteRemovedMetadataFiles`. The shared `delete_removed_metadata_files` helper diffs the base and committed metadata logs and deletes best-effort, logging failures without failing the commit. It is wired into the memory, SQL and Glue catalogs. REST and S3 Tables are left out because the server or service owns metadata file cleanup, and HMS does not support commits yet. Co-authored-by: Isaac <no-reply@databricks.com>
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are blocking the opt-in metadata cleanup change.
No open issues 🎉
What changed in this PR
Addresses #3581 by adding opt-in cleanup of old metadata files after successful commits in the memory, SQL, and Glue catalogs.
Changes:
- Adds
write.metadata.delete-after-commit.enabled, defaulting tofalse. - Introduces shared, best-effort cleanup of files removed from the metadata log, called only after a successful metadata pointer update.
- Adds tests for enabled and disabled cleanup, deletion failures, and cross-catalog behavior.
| File | Description |
|---|---|
crates/iceberg/src/spec/table_properties.rs |
Defines the cleanup property, getter, and default. |
crates/iceberg/src/lib.rs |
Exports the shared cleanup helper. |
crates/iceberg/src/catalog/utils.rs |
Implements bounded concurrent deletion with warning-only failures. |
crates/iceberg/src/catalog/memory/catalog.rs |
Runs cleanup after committing and releasing the lock; adds unit tests. |
crates/iceberg/public-api.txt |
Records the new property API and helper. |
crates/catalog/sql/src/catalog.rs |
Runs cleanup after a successful SQL catalog update. |
crates/catalog/loader/tests/table_suite.rs |
Tests cleanup across memory, SQL, and Glue catalogs. |
crates/catalog/glue/src/catalog.rs |
Runs cleanup after a successful Glue catalog update. |
Review effort: Balanced
|
cc @szehon-ho Can you take a look? Thanks! |
| } | ||
|
|
||
| /// Deletes the previous metadata files tracked by `base` that `new` no longer tracks, if | ||
| /// `write.metadata.delete-after-commit.enabled` is set on `new`. |
There was a problem hiding this comment.
Could we say “is true” here? The property can be explicitly set to false, which skips cleanup.
There was a problem hiding this comment.
Good point, changed it to "is true on new".
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_update_table_deletes_metadata_files_removed_from_log_when_enabled() { |
There was a problem hiding this comment.
As optional coverage, could we add a test that enables cleanup during an update and verifies deletion on that same commit? This would check that cleanup reads the new metadata’s setting.
There was a problem hiding this comment.
Added test_update_table_deletes_removed_metadata_files_on_commit_that_enables_cleanup. It creates the table with only previous-versions-max=1, commits twice with cleanup off so v0 falls out of the log, then sets delete-after-commit.enabled=true on the third commit. That commit deletes v1 and leaves v0 in place, since v0 fell out of the log while cleanup was off. This matches Java. The test fails if the helper reads the property from base instead of new. None of the existing tests caught that, because they all set the property at table creation.
Address review: test that cleanup reads the property from the new metadata, and say the property must be true in the helper's doc. Co-authored-by: Isaac <no-reply@databricks.com>
|
lgtm, thanks! |
|
Thanks @szehon-ho ! |
Which issue does this PR close?
What changes are included in this PR?
Adds the
write.metadata.delete-after-commit.enabledtable property (defaultfalse) and a sharediceberg::delete_removed_metadata_fileshelper that matches Java'sCatalogUtil.deleteRemovedMetadataFiles. Catalogs call it only after the metadata pointer swap succeeds, so a failed commit, or one whose outcome is unknown, never deletes anything.The helper reads the property from the new metadata (as Java does) and deletes the files in the base
metadata_logthat are not in the new one. It diffs the two logs rather than usingTableMetadataBuildResult::expired_metadata_logs, becauseTableCommit::applybuilds the metadata and drops that result before catalogs see it, and the diff is what Java does. Deletion is best-effort: each failure is logged withtracing::warn!and the commit still succeeds.Wired into the memory, SQL and Glue catalogs. Not wired into REST, where the server owns metadata files (Java doesn't delete client-side either), or S3 Tables, where the service manages file cleanup. HMS doesn't support
update_tableyet.Are these changes tested?
Yes.
falsekeeps everything; a delete that fails (a non-empty directory in place of the old file on the local FS) doesn't fail the commit, and later commits keep deleting.table_suiteincrates/catalog/loader: the same enabled scenario for the Glue, SQL and memory catalogs. I ran the Glue case locally against the moto and MinIO containers.AI Disclosure
Written with an AI assistant (Claude). Please check:
metadata_logby file path, and reading the property from the new metadata.