Remove parallel restore feature#12903
Conversation
…ompiles but untested.
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
|
This seems to be failing ctests. Moving to Draft 10/52 Test #10: BackupTests ....................................... Passed 0.00 sec |
|
Late adopter I realize but below is what the local AI says about this PR. It is not able to read the github comment since that overrides the warning about the old "compiles but untested" diff comment which it (reasonably) flags as item 6. This is either by local policy or by my own misconfiguration. Other than that there are some good ideas in here. On the dead code item (1 in list) it doesn't realize that the function using this knob (weightedTotalSize) itself is dead code. PR 12903 Review: Remove Parallel Restore Feature Base: fdb5 local repo (branch delete-parallel-restore, main at ~PR 12887) What It Does Completely removes the FastRestore/ParallelRestore subsystem — a distributed parallel backup restore implementation that was written but never shipped. Specifically deleted:
Intent This is dead-code removal. The parallel restore feature was written, had its own workers, knobs, and test suite, but was never production-ready. PR 12107 was an earlier attempt at this Potential Problems
The knob is declared in ClientKnobs.h:205 and actively used in CommitTransaction.h:121:
RestoreInterface.h declares keys (restoreRequestDoneKey, restoreRequestTriggerKey, restoreRequestKeys) and functions (restoreRequestTriggerValue, restoreRequestKeyFor) that are now dead
The UsePartitionedLog fastRestore parameter remains in the function signature. Any caller passing fastRestore=true will hit ASSERT(false) at runtime (in debug builds) or undefined
The enum value stays (with a static_assert preserving its numeric value of 11), and the switch case now hits ASSERT(false). This is intentional tombstoning of the enum value to avoid
If any running cluster (or a running simulation test from an older build) sends a worker with processClass == FastRestoreClass, the server will no longer handle it specially — it'll fall
No CI results are referenced. The FASTRESTORE_ATOMICOP_WEIGHT issue suggests it may not even compile cleanly. |
⏺ The fix restores a single if block that was accidentally deleted when removing the FASTRESTORE_TOOL code:
if (!restoreSystemKeys && !restoreUserKeys && backupKeys.empty()) {
addDefaultBackupRanges(backupKeys);
}
When no explicit key ranges are specified on the command line and neither --user-data nor --system-metadata flags are set, this populates backupKeys with the default backup ranges
(essentially all user data). Without it, backupKeys stays empty and hits the ASSERT(!backupRanges.empty()) in submitBackup().
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
|
Updated AI code review: Item 1 — Duplicate symbol: retracted. restoreRequestDoneKey is no longer in SystemData.cpp. It only exists in RestoreInterface.cpp. No duplicate. However RestoreInterface.cpp/.h are still Item 2 — fastRestore dead parameter: retracted. Reading the actual code, atomicRestore() no longer has a fastRestore parameter at all — the PR cleanly removed it. The ASSERT(false) branch Item 3 — FastRestoreClass tombstone: partially confirmed. The enum is marked // deprecated and the Locality.cpp switch hits ASSERT(false) then returns "fast_restore". Notably, the Summary: The two issues I flagged as significant (duplicate symbol, dead parameter) don't exist in the current code. The only real remaining item is the dead RestoreInterface.cpp/.h |
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
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-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 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-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-macos on macOS Ventura 13.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
|
saintstack
left a comment
There was a problem hiding this comment.
Skimmed. It is great. No references to fast restore in documentation?
|
@neethuhaneesha @akankshamahajan15 can you confirm none of this code is needed for the backup v3 stuff you are working on? |
Apparently not. There weren't any doc edits in the original PR I copied this from, AI didn't tell me about any, and a quick grep doesn't show anything. |
This parallel restore feature has been slated for removal for at least a year. This PR is closely based on earlier PR #12107.
This blog post explains some of the problems with the parallel restore feature: https://medium.com/@jingyuzhou/why-foundationdb-restore-is-slow-and-what-can-be-done-about-it-e73a821fdd33
As far as large feature removal changes go, this one is very straightforward, with most relevant files and test cases simply being deleted. There is one knob rename where storageserver.actor.cpp was using a knob with
FASTRESTOREin the name. Other than that, changes to shared files mainly involve removing fastrestore-specific CLI and role support.In progress:
20260330-222511-gglass-5ee0142213471b70 compressed=True data_size=35343375 duration=4611964 ended=100000 fail=1 fail_fast=1000 max_runs=100000 pass=99999 priority=100 remaining=0 runtime=0:58:23 sanity=False started=100000 stopped=20260330-232334 submitted=20260330-222511 timeout=5400 username=gglass
The one failure was in
SwizzledCycleTest.tomlwith too many lines of output and a timeout. I kind of suspect it's unrelated but haven't looked further.