Add size-based table retention - #19770

Reviewed the retention logic against the lineage/deletion paths it interacts with. The config plumbing, validation, and safety-skip behaviors (unknown sizes, missing metadata, REFRESH tables) all look solid, and the test coverage is thorough. Left a few inline comments — the main ones are about how lineage-related bytes are accounted (replaced segments and in-progress replacements can trigger eviction of live data) and about the silent-failure path when deleteSegments rejects the batch.

Re-reviewed the update (2e170fc). All five earlier comments are addressed well: replaced segments are excluded from accounting, an in-progress replacement now postpones the whole pass, the deleteSegments response is checked with correct byte accounting on failure, the newest OFFLINE segment is protected, hybrid REALTIME eviction is bounded by the offline time boundary, and the SIZE_RETENTION_BLOCKED gauge covers the observability gap. I also ran RetentionManagerTest at this head (46 tests, 0 failures) and the new tests are genuinely regression-sensitive — e.g. the lineage and newest-segment tests now assert the opposite of what the v1 tests asserted. Remaining feedback inline: one real issue (gauge cardinality for unconfigured tables) and a few minors.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Re-reviewed c32ac7f. All four round-2 comments are addressed cleanly:

• The disabled branch now calls removeTableGauge, so only tables with retentionSize configured carry the sizeRetentionBlocked series, and the ControllerGauge doc says "absent when disabled".
• The pass is refactored into enforceSizeBasedRetention with a single gauge write in a finally, so healthy passes never expose a transient blocked value, while an interrupting exception still publishes 1 (and there's now a test for exactly that path).
• One lineage snapshot is shared across the IN_PROGRESS check, replaced-segment filtering, and candidate protection, with the IdealState read deliberately ordered before the lineage read — I verified the invariant the comment claims: a replacement writes its IN_PROGRESS entry before its destinations enter ideal state, so a lineage snapshot without the entry guarantees the earlier IS snapshot has no destinations, and double-counting is impossible. testSizeRetentionSeesLineageStartedWhileReadingIdealState pins the race window directly. The flag-off residual race is now documented in the getRetentionSize javadoc.
• verifySizeRetentionGauge pins exactly one gauge write with the final value, and the real-registry lifecycle test covers register → blocked → recover → remove-on-disable → remove-on-leadership-loss end to end.

I ran RetentionManagerTest at this head on JDK 25: 49 tests, 0 failures, 0 errors. One take-it-or-leave-it nit inline; otherwise this looks ready to me.

Re-reviewed the three new commits. The lineage-barrier redesign is a real improvement over the full-pass IN_PROGRESS skip: eviction can now make progress on the old prefix during a replacement, every lineage member (any state) is protected by position rather than by flag-dependent filtering, and the pre-delete snapshot recheck under getLineageUpdaterLock closes the exclusive-delete-off TOCTOU raised earlier, within a controller (the cross-controller limitation is honestly documented). The round-3 nit is resolved by construction — the redundant getDeleteBlockedSegments call is gone and the barrier strictly subsumes it.

Things I verified rather than assumed:

• The snapshot-equality check is sound: LineageEntry has value-based equals/hashCode including state and timestamp, so freshly-parsed entries compare correctly (and testSizeRetentionRejectsPrefixWhenLineageStateOrTimestampChanges depends on exactly that).
• Shadow copies are excluded from accounting but still establish the barrier, and the barrier survives the last-LLC and hybrid-boundary candidate filters (the two CannotHideLineageBarrier tests pin this).
• The IdealState-before-lineage read order still prevents double-counting under the new model, since non-COMPLETED destinations are filtered from accounting.
• Ran RetentionManagerTest (62 tests) and ControllerZkHelixUtilsTest (2 tests) at this head on JDK 25: 0 failures, 0 errors.

Two inline notes — one operational question on barrier pinning, one process nit on the drive-by test fix. Nothing blocking from my side.

Re-reviewed the rebase plus 0690fde. Both round-4 comments are addressed and the scope issue resolved itself in the best possible way:

• The ControllerZkHelixUtilsTest drive-by fix was extracted into #19773 and merged separately, so this PR is back to cleanly retention-scoped files after the rebase.
• The new getRetentionSize paragraphs document exactly what I asked for: the barrier-pinning durations (4h/24h replaced-segment windows, 1d stale-IN_PROGRESS, the 6h retention schedule as an additional release delay, and the explicit "hours or longer at sizeRetentionBlocked=1" warning for merge-heavy tables), plus an explicit statement that the pre-delete snapshot check deliberately over-rejects.

I fact-checked every claim in the new javadoc against the code rather than taking it on faith: the 4h/24h defaults and all-ingestion-type applicability match DefaultLineageManager (the old getReplacedSegmentsRetentionPeriod doc was actually wrong on both counts — claiming APPEND ignored the setting and a 1d default — so these corrections fix real errors); REVERTED entries are indeed eligible immediately (state == REVERTED || short-circuits the age check); "unset, empty, or unparseable → default" matches getRetentionMsFromConfig; the 6h DEFAULT_RETENTION_MANAGER_FREQUENCY_PERIOD is right; and even the subtle two-pass timing ("entry is removed on a subsequent cleanup pass after sources leave IdealState") is accurate, since entry removal checks membership against the segment list captured at pass start.

Re-ran RetentionManagerTest on the rebased head (new master base) on JDK 25: 62 tests, 0 failures, 0 errors.

No remaining concerns — LGTM. (As the PR author I can't formally approve, but everything raised across the five review rounds is now resolved.)

A few non-blocking simplifications I’d suggest:

• Use long instead of BigInteger. Overflow would require nearly 8 EiB of compressed data in one table. We don’t need arbitrary-precision accounting or the synthetic overflow test for this use case.
• Simplify candidate filtering and use set membership. Filter the segment metadata directly instead of converting metadata → name list → name set → metadata. In particular, materialize the last-completed LLC names into a HashSet: the current ArrayList.removeAll(HashMap.values()) can take O(segments × partitions).
• Consolidate duplicate tests. The disabled-policy tests overlap, and the real-registry lifecycle test already covers deletion failure followed by successful retry. Keep the stronger lifecycle coverage and move any distinct assertions there. The null deletion-response case and corresponding guard can also go; the production API always returns a response.

The timestamp fallback makes sense for OFFLINE APPEND tables without a time column, so I’d keep it. I’m also fine keeping SIZE_RETENTION_BLOCKED for visibility into policies that cannot meet their cap.

Reviewed 89f6f0d. The simplification is almost entirely a win, and I verified the pieces that could have changed behavior:

• The candidateNames removal is semantically equivalent in all four filter branches — the ts≥0 pre-filter, last-LLC exclusion, hybrid boundary (endTime < 0 || >= boundary removal matches the old endTime >= 0 && < boundary retain), and newest-OFFLINE removal all select the same victims as before, and the barrier logic is untouched since it reads lineageOwnedMetadata independently.
• Dropping the response == null guard is safe: deleteSegmentsInternal returns a non-null response on every path (the whole body is wrapped in try/catch returning success/failure), and the test helper stubs SUCCESS so no mock default leaks through.
• The test consolidation into testSizeRetentionGaugeRegistryLifecycle preserves the failure→blocked→retry→recovered coverage against a real metrics registry, now in both REFRESH and unconfigured variants.
• Ran RetentionManagerTest at this head on JDK 25: 58 tests, 0 failures, 0 errors.

One real regression inline: the BigInteger→long change removes the overflow protection and its test, and the failure mode isn't benign — see the comment for the one-line way to keep both the simplicity and the safety.

Reviewed 3556208 — this resolves the round-6 overflow finding exactly as suggested: Math.addExact keeps the long simplicity while restoring the fail-safe behavior (warn + skip + sizeRetentionBlocked=1 on corrupt EB-scale sizes instead of silently reporting healthy), and the restored test now asserts the skip semantics for both OFFLINE and REALTIME. Ran RetentionManagerTest at this head on JDK 25: 59 tests, 0 failures, 0 errors.

Nothing further from me — every finding across the seven review rounds is resolved. LGTM.

Reviewed d521496. This cleanly reverts the Math.addExact guard and the synthetic overflow test, per the author's stated decision that totals beyond Long.MAX_VALUE are outside realistic table sizes. The resulting RetentionManager.java and RetentionManagerTest.java are byte-identical to the 89f6f0d state, which I already verified and ran green (58 tests, 0 failures on JDK 25), so there's nothing new to re-verify.

I'll register the residual trade-off once for the record and then consider it settled: the other corrupt-metadata shapes in this pass (negative size, missing metadata, unlocatable barrier) fail safe with a WARN and sizeRetentionBlocked=1, while a wraparound-scale corrupt sizeInBytes now reports the table healthy. Accepting that is a legitimate call — the input requires a segment claiming exabytes in ZK — and it's the author's to make. One small inline ask remains from round 6's fallback option; otherwise this is still LGTM.

Reviewed 7ee85f7. This reverses the hybrid time-boundary guard in favor of the "document it" option from the round-1 comment, and I think it's the right call on reflection: the guard made a hybrid REALTIME table's size cap silently unenforceable whenever the OFFLINE boundary couldn't resolve (broker RPC down, schema/time-column issues), coupling the REALTIME table's disk-safety mechanism to cross-table health. An operator who sets a size cap is stating a resource bound; not enforcing it because an unrelated lookup failed was the worse failure mode, and the gauge sat at 1 with no way to make progress.

Verified on this head:

• The removal is complete and clean — getHybridTimeBoundaryMs remains in use by hybrid time retention only, no stale imports in main or test code (checkstyle ran clean this time, not skipped).
• The rewritten tests assert the decoupling properly: getOfflineTableConfig and getTimeBoundaryInfo are verified never-invoked on the size path, in both hybrid-strategy settings, and the boundary-unavailable scenario now evicts and clears the gauge instead of blocking.
• The javadoc states the new semantics honestly, including that eviction order for undated-endTime segments falls back to creation/push time (the updated hybrid test pins invalidEndTime evicting first by its creation time).
• RetentionManagerTest: 57 tests, 0 failures, 0 errors on JDK 25.

One inline note on the operator-facing consequence; otherwise LGTM.

Reviewed 4292097 — a one-line, comment-only change that resolves the round-8 ask exactly: the plain-addition line now states the accepted wraparound trade-off in place, so future readers won't mistake the asymmetry with the other fail-safe guards for an oversight or re-add the rejected checked addition. No code or test behavior changed, so nothing to re-run.

The only item still open is the optional round-9 doc suggestion (hybrid sizing guidance in the retentionSize javadoc — "size the cap so the REALTIME table holds all data newer than the boundary"). That's a take-it-or-leave-it improvement, not a blocker. Everything else across all rounds is resolved — LGTM.

Final pass on the updated PR description (no code changed since 4292097). The rewrite now describes the shipped design rather than the v1 design, and I verified it the same way as the code rounds — checking claims against the implementation, not just reading them:

• Eviction rules 1–6 are accurate against the current head: IdealState-before-lineage read order, single planning snapshot, IN_PROGRESS no longer skipping the table, shadow-copy exclusion from accounting but not from barrier placement, newest-OFFLINE and last-LLC protections, the timestamp fallback chain with name tie-break, zero-byte skip, the under-lock full-map recheck (including its deliberate over-rejection), and the response check before accepting reductions.
• The monitoring section matches the gauge's actual lifecycle (single publish per pass including exceptions, absent when disabled, removed on leadership loss/table deletion).
• The cleanup-window paragraph is correct, including the subtle point that lineageEntryCleanupRetentionPeriod does not add a one-day delay to COMPLETED entries — those release on the pass after their sources leave IdealState.
• The validation claim holds with margin: I ran all seven named classes in full at this head on JDK 25 — RetentionManagerTest 57, TableConfigUtilsTest 84, RebalanceCheckerTest 7, TableConfigBuilderTest 5, SegmentLineageCleanupTest 3, ControllerZkHelixUtilsTest 2, ControllerGaugeTest 1 — 159 tests, 0 failures, 0 errors, 0 skips, a superset of the 78 targeted tests cited.
• The #19773 attribution and release note are accurate.

All three items from the maintainer's simplification review (plain long, in-place filtering with HashSet last-LLC lookup, test consolidation + null-guard removal) are confirmed addressed at this head as well. The only open item remains the optional round-9 hybrid sizing sentence for the javadoc — non-blocking. From my side this PR is complete and consistent: code, tests, docs, and description all describe the same system. LGTM.

Reviewed 7c1945b — a three-line javadoc addition that resolves the last open item (the round-9 hybrid sizing guidance): operators now get the actionable rule (size the REALTIME cap to retain everything newer than the routing boundary) plus the concrete failure mode if they don't (data deleted ahead of RealtimeToOffline, queries silently returning no rows from either side). Doc-only change, nothing to re-run.

With this, every item raised across all review rounds is resolved and nothing is outstanding. LGTM — ready to merge from my perspective.

LGTM. Re-reviewed 7c1945b; no blocking findings. The independent hybrid size-retention behavior is documented and covered by tests. All 13 reported CI checks passed.

One optional performance cleanup inline.

Reviewed b53e33f3. Clean refinement: the newest-OFFLINE-segment removal previously went through List.remove(Object), which invokes SegmentZKMetadata's value-based equals — a full toMap() materialization and comparison per scanned element — when the intent was "remove this exact instance". The identity-based removeIf is both semantically precise and cheaper. I checked the correctness angle: the old form wasn't actually buggy (segment names are unique within the list, so deep equality could only ever match the selected instance), so this is a robustness/efficiency improvement rather than a bug fix, and no behavior changes.

RetentionManagerTest at this head on JDK 25: 57 tests, 0 failures, 0 errors. Still LGTM — nothing outstanding.