feat(transaction): prune statistics files for expired snapshots#2667
feat(transaction): prune statistics files for expired snapshots#2667dhruvarya-db wants to merge 1 commit into
Conversation
When ExpireSnapshotsAction expires snapshots, also drop the statistics and partition-statistics metadata entries tied to each expired snapshot, mirroring Java RemoveSnapshots.removeSnapshots. This is metadata-only: the puffin files those entries point at are deleted by the higher-level file-cleanup operation.
viirya
left a comment
There was a problem hiding this comment.
Verified the design holds up. The key subtlety: commit() builds the updates vec and hands it straight to ActionCommit::new(...), so these RemoveStatistics / RemovePartitionStatistics updates do not go through TableMetadataBuilder::remove_statistics (which is itself only-when-present, gated on if previous.is_some()). That means the explicit statistics_for_snapshot(...).is_some() / partition_statistics_for_snapshot(...).is_some() guards here are necessary, not redundant — without them, expiring a snapshot with no statistics would emit a spurious RemoveStatistics. This matches Java RemoveSnapshots, which records a removal only when an entry exists.
test_only_present_statistics_variant_is_removed pins exactly that invariant (snapshot with statistics but no partition-statistics → only RemoveStatistics emitted), and the three tests together cover present-both / present-neither / present-one. Metadata-only scope (puffin file deletion left to file-cleanup) is the right boundary and mirrors Java's FileCleanupStrategy split.
Pulled the branch and ran it: all 32 transaction::expire_snapshots tests pass, clippy + rustfmt clean.
LGTM. (Note this stacks on #2591, which should land first.)
Which issue does this PR close?
Part of #2145. Extends #2591 (the
ExpireSnapshotsAction) and #2664 (thehistory.expire.*defaults).What changes are included in this PR?
When
ExpireSnapshotsActionexpires snapshots, it now also drops the statistics and partition-statistics metadata entries tied to each expired snapshot, mirroring JavaRemoveSnapshots(TableMetadata.Builder.removeSnapshots, which callsremoveStatistics/removePartitionStatisticsfor each removed snapshot). For every expired snapshot that has a statistics entry,commit()emits aRemoveStatisticsupdate; likewiseRemovePartitionStatisticsfor a partition-statistics entry. Only entries that actually exist produce an update, matching Java's behavior of recording a change only when an entry is present.This is metadata-only, physical deletion of the puffin files those entries point at is part of the file-cleanup maintenance operation (mirroring Java's
FileCleanupStrategy), not this PR.Are these changes tested?
Yes. New unit tests cover: an expired snapshot's statistics and partition-statistics entries are removed while a retained snapshot keeps its own; an expired snapshot with no statistics emits no removal; and only the statistics variant that is actually present is removed.