Allow restored data moves to split ordinary relocations#13650
Conversation
tclinkenbeard-oai
left a comment
There was a problem hiding this comment.
Generated by Codex.
What is it trying to do?
Allow a restored data move to split an overlapping ordinary in-flight relocation. The residual ordinary fragments are relaunched normally, while partial restored-over-restored overlap remains rejected.
Is it correct?
Yes. In the affected-range loop, the incoming restored move retains its original DataMove and ID only for its exact range. Other affected fragments reset dataMove and receive a fresh ID. The new assertion precisely permits an ordinary residual fragment while continuing to reject a residual restored fragment.
I inspected the changed path, restore construction and launch flow, in-flight cancellation/relaunch behavior, and downstream restored-move handling. There are no serialization or compatibility changes, no added hot-path work, and no altered async lifetime behavior.
No build or test validation was run as part of this review. GitHub’s clang-format, clang-tidy, and Windows Boost checks are green; the FoundationDB builders and cluster tests are still pending.
Are there bugs?
I did not find any correctness bugs.
Are there omissions?
No blocking omissions. A dedicated regression test could make the restore-over-ordinary split case more explicit, but the PR identifies the existing simulation workload and exact failing tuple that exercised the assertion.
Are there better ways of doing things?
The localized assertion change is the right scope. Expanding this into broader relocation-queue restructuring would add risk without improving the fix.
Should this CL be LGTMd?
Yes, LGTM from code inspection. The highest remaining risk is whether the reported simulation reproduction fully covers the intended overlap shape; final landing should still wait for the pending public CI contexts.
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 on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS 14.x
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-macos on macOS 14.x
|
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-clang-arm on Linux RHEL 9
|
Result of foundationdb-pr-macos on macOS 14.x
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm 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 on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Test failure:
tests/fast/DDPipelineSaturation.tomlhit a simulator assertion because a restore relocation reached a path that forbade restore moves.Bug: A simulation assertion rejected a restored data move that legitimately split an ordinary in-flight SizeSplit relocation.
Fix: Reject only partial restored-over-restored overlap; preserve cancel, split, and relaunch for ordinary fragments.
Validation: The exact tuple passed through simulated time 417.792 versus the original assertion at 102.710699; the combined same-base replay preserved the pre-failure trace and passed; 43/43 data-distributor tests passed.