Repository navigation
Conversation
Add a passes counter by anchor type, data group and outcome, and the last clean pass time per type and group, for the delta compactor to report into. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZANJ6GDMbc1ZaTeoBVXii
Each scan plan (anchor type, data group) now tallies a pass from the first page of its prefix to the page that reaches the end: anchors scanned, migrated, skipped as not led here, and failed to build. When the pass ends the migrator logs "ttl inline migrator: pass complete" with stable keys and reports it to the observer main wires from the metrics registry. A pass is clean when it started at the top of the prefix in this leadership term and migrated, skipped and failed nothing. A plan that drops out of the tick's plans (its group's leadership moved) loses its open pass but keeps its cursor, so the pass it resumes is not full and cannot be clean. A page retried after a failed batch counts once, plus the keys its committed batches rewrote. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZANJ6GDMbc1ZaTeoBVXii
Document the pass report and the condition to check before disabling the legacy !redis|ttl| read fallback, and what a clean pass does not cover. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZANJ6GDMbc1ZaTeoBVXii
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 30 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
CI note: This PR doesn't cause it: the diff touches only Generated by Claude Code |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41548be683
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if state.pass == nil || (fromStart && state.pass.pages > 0) { | ||
| state.pass = &ttlInlineMigrationPass{full: fromStart, startedAt: time.Now()} |
There was a problem hiding this comment.
Inferring full solely from fromStart does not ensure that the pass stayed within one leadership term as claimed. If a group loses and regains leadership entirely between two compactor ticks, pruneTTLInlineMigrationPlanStates never observes its absence, so the existing open pass remains full; the node then resumes after its old cursor and may emit a clean pass without rescanning keys changed while another leader was active. This can incorrectly authorize removal of the legacy fallback, especially during a rolling deployment where the intervening leader can still write legacy anchors. Associate pass state with a leadership generation or restart the pass when continuity cannot be established.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not fixed in code. I corrected the claim in the doc (9eb1c1f) and made the precondition explicit instead.
What I traced. A group can lose and regain leadership between two ticks without the migrator noticing. The pass then stays open and full. Every anchor behind its cursor was still read inline earlier in this pass. For one of them to be legacy now, the interim leader must have written the legacy format. That needs a live build older than Phase 1. If such a writer exists, no pass can gate Phase 3 anyway: it could write a legacy anchor behind the cursor of any pass, even after a perfect one. So the gate needs "no live legacy writer" either way.
What changed.
- Gate condition 1 now says this explicitly.
full_passno longer claims "one leadership term". The doc says it means the group stayed in every tick's plans, and that a lose-and-regain between ticks is not noticed. "What a clean pass does not prove" explains why that's safe given condition 1.
Why not the code fix. Tracking leadership terms would need a new coordinator capability, such as per-group Raft terms. That method would have to be threaded through ShardedCoordinator, CoordinatorForwarder and the wrapper guard tests. For a case condition 1 already rules out, that's more code than it buys. I've passed the call to the PR author, @bootjp.
Generated by Claude Code
…m their first scan Codex review on #1283 found two defects in the pass tally: - Two ticks that read the same cursor both added the same page to the pass. On a middle page the pass counted it twice; on the last page the second tick opened and reported a spurious non-full pass. A page is now dropped when the plan's cursor moved after its scan began. - A pass started when its first page was finished, so its duration left that page out. It now starts when the first page's scan begins. Both carry a regression test that failed before the fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZANJ6GDMbc1ZaTeoBVXii
…ot the last clean pass Codex review on #1283: a last-clean-pass gauge keeps its value after a later not_clean pass, and max across nodes can pick a former leader's stale clean sample. Either let the Phase 3 gate pass while the current leader was still finding legacy anchors. Passes now report one of three outcomes: clean, partial (found nothing but did not read the whole prefix) and not_clean. The gauge becomes last_pass_timestamp_seconds{type,group,outcome}. The doc's gate requires the latest clean pass for every type and group to be later than T0 (the last upgrade, route move between groups, or restart) and later than every not_clean pass on any node. The doc also states that a leadership loss and regain between two ticks is not noticed. It says why that cannot make a clean pass wrong while no live build writes the legacy format, and lists that as an explicit gate condition. The tests were updated first to the new outcome API and failed to build before the change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZANJ6GDMbc1ZaTeoBVXii
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9eb1c1f791
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review on #1283: a pass that ends after T0 may have read its first pages before T0. A route change at T0 can then put a legacy anchor behind its cursor, and the remaining pages still finish clean. The observer now receives the pass's start, which is when its first page's scan began. The gauge becomes last_pass_start_timestamp_seconds{type,group,outcome}. The doc's gate now requires the latest clean pass to have started after T0 and after every not_clean pass. The updated tests (the gauge value and the start the observer receives) failed to build before the change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZANJ6GDMbc1ZaTeoBVXii
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aaee3d9998
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review on #1283: SyncOnce acquires readTS at the start of the tick, but a pass's start was stamped only when its first page's scan began, after the delta handlers had run. If a route moved in between, a pass that read a snapshot from before the move reported a start after it, so it could satisfy the Phase 3 gate's started-after-T0 condition. SyncOnce now notes the time before it acquires the read timestamp and hands it to every page of the tick. A pass opened by such a page starts then. Regression test: the reported start precedes the coordinator's first Clock() call, which is where the read timestamp is taken. Before the fix it came about 10ms after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZANJ6GDMbc1ZaTeoBVXii
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91df14c46f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review on #1283 found three more ways for overlapping ticks to corrupt pass reports: - a failed stale page still added its migrated keys; - reports from overlapping ticks could reach the gauge out of order; - on a one-page plan the cursor resets to empty, so a stale page passed the cursor check and was reported as a second pass (ABA). All three come from ticks overlapping, which only happens when SyncOnce is called concurrently (tests); Run calls it serially. The migration now holds ttlInlineMu for the whole tick, and a tick that finds it held skips the migration. That makes the per-page cursor check from 4e2a406 redundant, so it is removed. Regression test: TestDeltaCompactor_TTLInlineTicksDoNotOverlap runs a tick inside the last page's scan, for a one-page plan and a middle page. Before the change the overlapping tick scanned again, and the one-page plan reported two passes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZANJ6GDMbc1ZaTeoBVXii
Codex review on #1283: pass starts come from each node's wall clock, so a node whose clock runs ahead can record a pass that began before T0 as starting after it. The gate's comparisons, start after T0 and clean after not_clean, now require a margin of S, the deployment's guaranteed maximum clock offset. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZANJ6GDMbc1ZaTeoBVXii
2 participants
Design: docs/design/2026_04_17_implemented_ttl_inline_value.md (Phase 3)
Builds on #1278 (now merged):
ScanGroupOwnedAtpages and plan rotation.Summary
There was no way to tell when the Redis TTL-inline migration had drained its backlog. A decision to disable the legacy
!redis|ttl|read fallback (Phase 3) could only be based on elapsed time. This PR makes the migrator report each completed pass, per anchor type and data group, and documents the condition to check before disabling the fallback.What a pass is
ttlInlineMu). A tick that finds another migrating skips the migration.What each pass reports
At the end of each pass the migrator logs
ttl inline migrator: pass completewith stable keys:type,group_id;clean,full_pass,pages;keys_scanned,keys_migrated,keys_skipped,keys_failed;duration.cleanrequires all of the following:full_pass: the pass started at the top of the prefix, and its group stayed in every tick's plans on this node since;Log levels:
Metrics
Each pass has an outcome:
clean;partial: rewrote, skipped and failed nothing, but was not full;not_clean.Two metrics, labelled
type,groupandoutcome, wired from the metrics registry inmain.go:elastickv_redis_ttl_inline_migration_passes_total;elastickv_redis_ttl_inline_migration_last_pass_start_timestamp_seconds: when the latest pass with that outcome began.Edge cases
LocalLeaderGroupIDsloses its open pass on the next tick, but its cursor is kept. If the group comes back, the resumed pass ispartial. Losing and regaining leadership between two ticks is not noticed; the doc explains why that is safe given gate condition 1 (no live legacy writer).Phase 3 gate (doc)
T0 is the latest of: the last upgrade, the last route move between groups, and the last restart of any node. S is the deployment's guaranteed maximum clock offset. Then:
not_cleanpass on any node. A PromQL query in the doc must return nothing.The doc also lists what a clean pass does not cover:
Behavior change
SyncOncethat overlaps another one's migration now skips the migration. Overlap only happens with concurrentSyncOncecalls, a test-only path;Runcalls it serially.scanTTLInlineMigrationRawreturns a page struct.migrateTTLInlinePage.advanceCursorgained a locked variant that reports a reset.migrateTTLInlineOncetakes the tick's snapshot time.newRedisDeltaCompactorIfEnabledtakes the metrics registry.Risk
Low; the change is observability only.
TryLockper tick and one morecursorMuacquisition per page.Review rounds (Codex)
TestDeltaCompactor_TTLInlineTicksDoNotOverlapTestDeltaCompactor_TTLInlineTicksDoNotOverlap(both subtests)durationomits the first pageTestDeltaCompactor_TTLInlinePassDurationIncludesFirstPageTestDeltaCompactor_TTLInlinePassStartsNoLaterThanItsReadTimestampfull_passTest evidence
TestDeltaCompactor_TTLInlineReportsCompletedPasses(table-driven,adapter/redis_delta_compactor_test.go):partial, then clean);monitoring/redis_ttl_inline_test.go: counters, start-time gauges and nil safety.go test -race -count=1 ./adapter/... ./monitoring/...passes on 33ab941 (adapter 227s).TestRedis_BullMQDirectCommandsfailing withNOTLEADER leader not found, an election-timing failure in a test this PR doesn't touch. It passed 8/8 on its own.go test -run 'Background|DeltaCompactor|TTLMigration|Capabilit' .passes../...:0 issues.TestDeltaCompactor_TTLInlineStopsAfterTimeout(1ms tick budget) failed 2 in 3000 race runs, both with and without this change.Self-review (five lenses)
readTS; the cursor still holds on a dispatch error. An overlapping tick skips the migration instead of repeating work.TryLock, soSyncOncenever blocks on it).cursorMu.partial.TryLock, onetime.Now()and a map prune.not_cleanpasses, with a clock-skew margin.🤖 Generated with Claude Code
https://claude.ai/code/session_01YZANJ6GDMbc1ZaTeoBVXii