Finish storageserver.actor.cpp coroutine conversion#13384
Conversation
tclinkenbeard-oai
left a comment
There was a problem hiding this comment.
Generated by Codex.
What is it trying to do?
Finish converting the storage server implementation from Flow actors to standard C++ coroutines, rename storageserver.actor.cpp to storageserver.cpp, replace the large request-multiplexing race in storageServerCore with dedicated serving coroutines, and add coro::FrameSizeRecorder so watch memory accounting uses coroutine frame sizes. It also updates affected documentation references.
Is it correct?
I did not find a behavior-changing correctness defect in code inspection. I compared the converted future-version waits, watch lifecycle/timeout paths, shard-state timeout, updateStorage durable-commit loop, storageServerCore dispatch, and replaceInterface rejoin loop against their actor versions. The converted locals remain live across awaits, timeout winner ordering matches the old choose ordering, and the moved handlers preserve the old reply/error branches. The patch does not change serialized messages, durable records, status output, or another compatibility boundary.
This was code inspection only; I did not build or run tests. At review time, clang-format and Test Boost CONFIG Mode on Windows were green; seven FoundationDB builder/cluster checks were still pending and no check was failing.
Are there bugs?
I did not find any correctness bugs.
Are there omissions?
There is no focused regression coverage for the watch timeout/reference-count cancellation path or the moved request-dispatch paths. I do not think that alone blocks this conversion, but it is the main residual risk; the pending full builders and cluster tests should finish before merge.
Are there better ways of doing things?
A small test with two watches on the same key where one times out while the other remains, plus a shard-state timeout/handler-dispatch test, would directly exercise the riskiest equivalence points. I would not require a different implementation.
Should this CL be LGTMd?
Yes, LGTM on code inspection, contingent on pending CI finishing green. The highest remaining risk is watch cancellation/reference-count behavior because it lacks focused regression coverage.
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS 14.x
|
Result of foundationdb-pr-macos on macOS 14.x
|
Result of foundationdb-pr-clang-arm on Linux RHEL 9
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
This is a valid concern, pushed a new fix to address it |
…rageserver-coroutines-20260622 # Conflicts: # fdbserver/storageserver/storageserver.cpp
Result of foundationdb-pr-clang-arm on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux RHEL 9
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux RHEL 9
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-clang 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-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS 14.x
|
Result of foundationdb-pr-macos on macOS 14.x
|
Result of foundationdb-pr-macos-m1 on macOS 14.x
|
Result of foundationdb-pr-macos-m1 on macOS 14.x
macOS stat flags, not linux compatiblelock_mtime() { stat -f %m $LOCK || echo 0 }
|
Result of foundationdb-pr-macos on macOS 14.x
macOS stat flags, not linux compatiblelock_mtime() { stat -f %m $LOCK || echo 0 }
|
Result of foundationdb-pr-macos on macOS 14.x
|
Result of foundationdb-pr-macos on macOS 14.x
macOS stat flags, not linux compatiblelock_mtime() { stat -f %m $LOCK || echo 0 }
|
Result of foundationdb-pr-macos-m1 on macOS 14.x
macOS stat flags, not linux compatiblelock_mtime() { stat -f %m $LOCK || echo 0 }
|
Result of foundationdb-pr-macos-m1 on macOS 14.x
|
…rageserver-coroutines-20260622
Result of foundationdb-pr-clang-arm on Linux RHEL 9
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS 14.x
|
Result of foundationdb-pr 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
|


This PR uses the actor rewrite tool to convert
storageserver.actor.cppto standard coroutines.storageServerCoreis refactored to avoid a largeracestatement inside of a loop, and watch actor overhead is now calculated with a newFrameSizeRecorderutility.