Retire old TLog roles after terminal recovery - #13893
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
With repopulate_anti_quorum=1, storage recovery can finish before current remote TLogs have copied the preceding log generations. Keep that history in coordinator state until every lagging remote TLog has durable progress at the current local start version. Preserve STORAGE_RECOVERED separately so a lost region can still be removed. Cover partial recruitment, the real old-to-current cursor handoff, interrupted waits, and stale rejoin replies with focused regression tests.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
tclinkenbeard-oai
left a comment
There was a problem hiding this comment.
Generated by Codex.
What is it trying to do?
Retire old-generation TLog roles after recovery has durably reached a safe terminal state, avoiding another recovery solely to remove an excluded old TLog. Preserve old history while current remote TLogs still need it.
Is it correct?
Yes, by source inspection at 4436a6005bd82771ed04b46ce4e31b9eaf26ca50. The retention checks protect both core-state construction and incremental purging. The remote-prefix condition uses durable known-committed progress, handles stale interfaces and failures, and remains separate from STORAGE_RECOVERED. The terminal transition waits for the coordinator write before withdrawing old roles and publishing FULLY_RECOVERED.
I also checked current-TLog exclusions, shared-log cleanup, future ownership and wakeups, and serialization boundaries. No wire-format change or transaction hot-path work is introduced.
Are there bugs?
I did not find any correctness bugs. The previously identified premature remote-prefix retirement is addressed.
Are there omissions?
None that I think block this. The regressions exercise partial recruitment, one lagging member of a remote pair, the handoff boundary, actual cursor selection, cancellation, RPC errors, and stale replies.
The highest remaining risk is distributed controller/router failure during catch-up. A directed multiregion regression that interrupts that wait and checks subsequent recovery would strengthen coverage; the focused tests mock the metrics producer and coordinated-state write.
Are there better ways of doing things?
Reusing the existing metrics RPC and caching one recovery-owned future is a reasonable narrow design. The predecessor review's concerns are resolved or nonblocking: role retention now ends at terminal recovery, the logsKey/advertisement distinction remains bounded by durability, and reserialization stays on the recovery path. I would not broaden this change.
Should this CL be LGTMd?
Yes, LGTM from source review. Windows Boost, clang-format, and clang-tidy pass; seven FoundationDB build/cluster checks are still pending, with no failing current-head check. Complete normal CI before merging the draft.
I ran no builds, tests, or simulations for this review.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…nkenbeard/logsystem-streaming-fixtures-241bfcf6-pr13913
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS 14.x
|
This comment has been minimized.
This comment has been minimized.
Result of foundationdb-pr-clang-arm on Linux RHEL 9
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-macos on macOS 14.x
|
Result of foundationdb-pr on Linux RHEL 9
|
Review order
This is part 2 of the split. Review and land #13913 first.
The retirement-only diff is six files, +113/-35. Use that comparison for review while this PR's Files changed view remains cumulative against
main. After #13913 lands, merge updatedmaininto this branch if needed so Files changed narrows. The original published commits remain in the branch history.Summary
Keep the old-generation TLog availability protection from #13331 during partial recovery, but retire those roles after the ordinary recovery has durably written its terminal coordinator state. That state must contain every expected current log set and no old generations.
The retirement transition stops old rejoins and old-role advertisements without changing current transaction-system membership or clearing shared in-memory log data. Remove the
OldTLogExcludedrecovery trigger, which otherwise starts another recovery solely to retire an already-drained excluded TLog. Excluding a current TLog still triggers recovery.The remote-prefix durability barrier and its focused regressions now belong to #13913.
Tests
fdbserver_logsystem_test: 23 passed; simulated remote-prefix filter: 3 passed; simulated retirement filter: 1 passed.fdbserver_clustercontroller_test: 31 passed.GcGenerationsandKillRegionCyclesimulations passed, including anti-quorum-one configurations, with durable-write and retirement-ordering trace checks.