fix(table): read retention table properties as defaults in ExpireSnapshots#877
Merged
zeroshade merged 2 commits intoApr 13, 2026
Merged
Conversation
…shots ExpireSnapshots required the caller to always pass WithRetainLast and WithOlderThan; it never consulted the table-level retention properties (min-snapshots-to-keep, max-snapshot-age-ms, max-ref-age-ms), even though their keys and defaults are defined in properties.go. This meant tables that set these properties via a catalog would not honor them during snapshot expiry, diverging from the Java implementation. Read the three retention properties from table metadata and use them as the last-resort fallback in the cmp.Or chains for maxRefAgeMs, minSnapshotsToKeep, and maxSnapshotAgeMs. When a property is absent the existing math.MaxInt constants apply, preserving current behaviour for callers that pass explicit options. Adds TestExpireSnapshotsUsesTableProperties which calls ExpireSnapshots with no options and verifies that the table-property defaults are respected. Fixes apache#839 Signed-off-by: Ali <alliasgher123@gmail.com>
laskoviymishka
approved these changes
Apr 13, 2026
Contributor
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
Thanks for contribution! Good fix: ExpireSnapshots should read table-level retention properties as defaults, matching Java.
Two nits: removing the error returns is a silent behavioral change, and the test has a couple of rough edges.
…operties Setting max-ref-age-ms to its own default value had no effect and obscured the test's intent. The test now proves that an absent property correctly falls back to the math.MaxInt default, which is the actual behaviour being verified. Signed-off-by: Ali <alliasgher123@gmail.com>
Contributor
|
@tanmayrauth can you review this one, and close #883 as dupe? |
zeroshade
approved these changes
Apr 13, 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
3 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.
Summary
ExpireSnapshotsrequired callers to always passWithRetainLastandWithOlderThan; it never consulted the table-level retention properties (min-snapshots-to-keep,max-snapshot-age-ms,max-ref-age-ms), even though their keys and defaults are defined inproperties.go. Tables that set these properties via a catalog would not honor them during snapshot expiry, diverging from the Java implementation.Fix: read the three retention properties from table metadata and use them as the last-resort fallback in the
cmp.Orchains formaxRefAgeMs,minSnapshotsToKeep, andmaxSnapshotAgeMs. When a property is absent themath.MaxIntconstants apply, preserving current behaviour for callers that pass explicit options.Fixes #839
Test plan
go test ./...gofmt -lcleanTestExpireSnapshotsUsesTableProperties: creates a table withmin-snapshots-to-keep=2andmax-snapshot-age-ms=0set as properties, callsExpireSnapshots()with no options, and asserts exactly 2 snapshots survive.Signed-off-by: Ali alliasgher123@gmail.com