Core: Use transaction metadata for post-commit file cleanup - #17708
Open
raunaqmorarka wants to merge 2 commits into
Open
Core: Use transaction metadata for post-commit file cleanup#17708raunaqmorarka wants to merge 2 commits into
raunaqmorarka wants to merge 2 commits into
Conversation
BaseTransaction resolved its newly committed snapshots through ops.current(), a catalog re-read that can be stale when the catalog caches table pointers. A missing snapshot made committedFiles return null, skipping clean-up of all uncommitted files and leaking one stale manifest list per commit retry attempt. The transaction's own committed metadata always contains the new snapshots, so resolve them there.
uros-b
approved these changes
Aug 19, 2026
Member
|
+1, LGTM |
ebyhr
approved these changes
Aug 19, 2026
anoopj
reviewed
Aug 19, 2026
| // committedFiles returns null whenever the set of committed files | ||
| // cannot be determined from the provided snapshots | ||
| private static Set<String> committedFiles(TableOperations ops, Set<Long> snapshotIds) { | ||
| private static Set<String> committedFiles( |
Member
There was a problem hiding this comment.
Looks like the else block and the return null on line 476 will become unreachable? It's fine if we just want to keep the defensive check there regardless
Contributor
Author
There was a problem hiding this comment.
Right, the ids now come from iterating current.snapshots() and are looked up in the same immutable metadata object, so the null path could never fire. Rather than keep a dead branch, 27b96e1 collects the Snapshot objects directly and passes them to committedFiles, removing the null contract and the unreachable warn.
anoopj
approved these changes
Aug 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BaseTransaction resolves its newly committed snapshots through
ops.current(), a catalog re-read that can return stale metadata when the catalog caches table pointers. When any new snapshot is missing from the stale view,committedFilesreturns null and clean-up of all uncommitted files is skipped ("Failed to load metadata for a committed snapshot, skipping clean-up"). With concurrent writers on a Glue catalog this fired on every commit and leaked one stale manifest list per commit retry attempt.The transaction's own committed metadata always contains the new snapshots, so resolve them there.
The new test serves stale metadata from
current()after a successful commit with injected retries. Without the fix it fails because no uncommitted files are ever deleted.