Retain newest N versions / last X days in named-blob stale cleanup - #3293
Conversation
Today the active-container cleaner keeps only the single latest READY version of each named blob and deletes every superseded version immediately. That leaves no middle ground for consumers that still resolve older versions, so such containers get put on the cleanup exclusion list and then bloat without bound. Add two global config knobs consumed by getStaleBlobsForActiveContainer: - stale.data.retention.versions (default 5): keep the newest N versions per blob name; superseded versions ranked beyond N are cleaned up. - stale.ready.data.retention.days (default 180): clean up a superseded READY version older than X days regardless of the version count. 0 disables the age rule. A stale version is cleaned up when it is ranked beyond N OR older than X days; the current (latest) version is always retained regardless of age. This is strictly more retentive than the previous keep-only-latest behavior (it deletes a subset of what is deleted today), so it cannot delete anything currently kept. It gives a bounded, consumer-friendly retention window instead of the all-or-nothing keep-latest vs exclude-entirely choice. IN_PROGRESS handling is unchanged. Adds unit tests for keep-newest-N, age-based cleanup, latest-always-kept, and the N=1 legacy behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3293 +/- ##
=============================================
- Coverage 64.24% 50.72% -13.53%
+ Complexity 10398 8688 -1710
=============================================
Files 840 938 +98
Lines 71755 80587 +8832
Branches 8611 9707 +1096
=============================================
- Hits 46099 40876 -5223
- Misses 23004 36311 +13307
- Partials 2652 3400 +748 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…sions=1) The cleanup-scenario integration tests assert the keep-only-latest behavior. Set stale.data.retention.versions=1 and stale.ready.data.retention.days=0 in the test base so the new production defaults (keep newest 5 versions / 180 days) do not change their expected stale counts. The retention policy itself is covered by the MySqlNamedBlobDbTest unit tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The two day-based retention configs are easy to confuse. Document that stale.data.retention.days governs IN_PROGRESS (incomplete upload) versions and stale.ready.data.retention.days governs superseded READY (completed) versions, with cross-references, plus a differentiating comment at the cutoff computation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
satishkotha
left a comment
satishkotha
left a comment
There was a problem hiding this comment.
Reviewed the current scope (retention-policy config + getStaleBlobsForActiveContainer + tests). The core change is correct and durability-positive: the new policy is strictly more retentive than the previous "keep only the latest READY version" behavior, and the current/latest version is provably never marked stale (it is only ever keepBlob, and only IN_PROGRESS keepBlobs are added). Nice, thorough config Javadoc. A few in-scope notes below — the main one is confirming the fleet-wide default change is intended/ramped; the other two are a pagination-interaction question and a test-coverage suggestion.
| * (retain stale versions by count only). Default 180. | ||
| */ | ||
| @Config(STALE_READY_DATA_RETENTION_DAYS) | ||
| @Default("180") |
There was a problem hiding this comment.
These defaults (stale.data.retention.versions=5, stale.ready.data.retention.days=180) flip the effective stale-cleanup policy fleet-wide on upgrade: getStaleBlobsForActiveContainer now returns far fewer superseded READY versions as stale (keeps the newest 5 + everything younger than 180 days), whereas before it returned every non-latest READY version. That is safer for durability, but it also means the downstream cleanup deletes fewer blobs, so both named_blobs_v2 row counts and the underlying blob storage will grow. Since this isn't gated behind a feature flag, can we confirm the storage/table growth is intended and captured in the PR's Risk/Observability/Rollout sections, and that a per-fabric ramp is planned rather than an all-at-once flip? (Legacy behavior remains available by pinning versions=1 / ready.days=0, as the integration base now does.)
There was a problem hiding this comment.
Good call. I flipped the defaults to the legacy behavior — stale.data.retention.versions=1 and stale.ready.data.retention.days=0 — so an upgrade doesn't change cleanup on any fabric. The 5-version / 180-day policy is now the documented per-fabric ramp target, not a default.
That also fixes the ordering you'd otherwise hit: with a legacy default you ramp forward per fabric, instead of having to first override every fabric back to 1/0. Storage and named_blobs_v2 growth are only incurred where a fabric opts in, so they can be watched at each ramp step. 715e419
| } else if (keepBlobState == NamedBlobState.READY && currentBlobState == NamedBlobState.READY) { | ||
| staleBlobs.add(currentBlob); | ||
| readyVersionsSeen++; | ||
| boolean beyondVersionLimit = readyVersionsSeen > versionsToKeep; |
There was a problem hiding this comment.
Question on the interaction with pagination: readyVersionsSeen is counted per invocation, but the caller pages the container via getAllBlobsForContainer (LIMIT queryStaleDataMaxResults, default 1000). If one blob name has more READY versions than fit in a single page, this call sees only a prefix and the count restarts at 1 on the next page. I believe that is safe — the page cursor is blob_name >= ... and already-cleaned rows are filtered by deleted_ts, so each subsequent page re-ranks from the newest surviving version and converges; the worst case is extra passes / temporary over-retention, never over-deletion. Can you confirm that's intended, and maybe add a one-line comment that the ranking is per-page (not whole-blob-name) so a future reader doesn't assume otherwise?
There was a problem hiding this comment.
Confirmed — that's intended, and your reasoning is right. The rank is per page: because the cursor is blob_name >= ... and already-cleaned rows drop out via the deleted_ts filter, each subsequent page re-ranks from the newest surviving version and converges. The newest N are always rank 1..N, so they're never marked stale — worst case is a few extra passes / temporary over-retention, never over-deletion.
Added a comment at readyVersionsSeen spelling out the per-page ranking and the convergence, so it isn't mistaken for whole-blob-name ranking. 715e419
| } | ||
|
|
||
| @Test | ||
| public void testStaleRetentionKeepsNewestNVersions() { |
There was a problem hiding this comment.
These new tests are a good start but only exercise a single blob name with uniform READY state. Consider adding two cases that cover the subtler branches of the new logic: (1) a list with multiple distinct blob names, to prove readyVersionsSeen resets at the name boundary (so the Nth version of blob a doesn't leak into blob b's count); and (2) READY and IN_PROGRESS versions interleaved under one name, to prove the counter only advances on READY and that the READY→IN_PROGRESS / IN_PROGRESS→READY transitions still behave. Those are the paths most likely to regress and are currently uncovered.
There was a problem hiding this comment.
Added both:
testStaleRetentionCountResetsAcrossBlobNames— two names, 3 READY versions each, N=2: only the 3rd of each name is stale. If the rank leaked across the name boundary,b's versions would be ranked beyond 2 and wrongly cleaned, so this pins the reset.testStaleRetentionCounterOnlyAdvancesOnReadyVersions— READY/IN_PROGRESS interleaved under one name: v3 stays rank 2 and is kept while the IN_PROGRESS versions are cleaned, proving the counter only advances on READY.
…d tests - Default stale.data.retention.versions=1 and stale.ready.data.retention.days=0 so an upgrade preserves the legacy keep-only-latest behavior fleet-wide; the 5-version / 180-day policy is now an explicit per-fabric ramp target (documented on the config fields). This is what makes a staged ramp possible instead of an all-at-once flip. - Comment that the READY rank in getStaleBlobsForActiveContainer is per-page (not per whole blob name): it converges across pages via the deleted_ts filter and the blob_name cursor, and never over-deletes. - Add unit tests for the rank reset across blob names and for interleaved READY/IN_PROGRESS versions (the counter only advances on READY). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3 participants
Summary
Give the named-blob stale-data cleaner a bounded, configurable retention window instead of the current all-or-nothing behavior (keep only the latest version, or exclude the container entirely and let it bloat).
Today
getStaleBlobsForActiveContainerkeeps only the single latest READY version per blob name and deletes every superseded version immediately. Consumers that still resolve older versions therefore have to be added to the cleanup exclusion list — and those containers then bloat without bound.Change
Two global config knobs (on
MySqlNamedBlobDbConfig), consumed by the cleaner:stale.data.retention.versions(default 5): keep the newest N versions per blob name; superseded versions ranked beyond N are cleaned up.stale.ready.data.retention.days(default 180): clean up a superseded READY version older than X days regardless of the version count.0disables the age rule.Rule: a stale version is cleaned up when it is ranked beyond N OR older than X days. The current (latest) version is always retained regardless of age. IN_PROGRESS (incomplete-upload) handling is unchanged.
Safety
This is strictly more retentive than the previous keep-only-latest behavior — it deletes a subset of what is deleted today — so enabling it cannot delete anything currently kept. It composes with the existing keyset pagination and resume cursor (a bloated blob converges toward N over runs).
Testing
Unit tests for: keep-newest-N, age-based cleanup (>X days), latest-always-kept-even-if-old, and the N=1 legacy behavior.