Remove IKnobCollection

[tclinkenbeard-oai]

This PR is a follow-up to #12683. The *KnobCollection hierarchy was necessary for the dynamic knobs implementation, but it's now unnecessarily complex and can be removed. The main benefits of this PR are:

• Avoids virtual dispatch when reading knobs (consumed ~2% CPU in a simple mako benchmark I ran prior to this change)
• Allows server knobs to move to fdbserver/core, rather than being part of fdbclient

Code-Reviewer Section

The general pull request guidelines can be found here.

Please check each of the following things and check all boxes before accepting a PR.

• The PR has a description, explaining both the problem and the solution.
• The description mentions which forms of testing were done and the testing seems reasonable.
• Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

• This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or main if this is the youngest branch)
• There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

[foundationdb-ci]

Result of foundationdb-pr-clang-ide on Linux RHEL 9

• Commit ID: c257113
• Duration 0:27:04
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

• Commit ID: c257113
• Duration 0:33:30
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang-arm on Linux CentOS 7

• Commit ID: c257113
• Duration 0:42:02
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-macos on macOS Ventura 13.x

• Commit ID: c257113
• Duration 0:47:25
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang on Linux RHEL 9

• Commit ID: c257113
• Duration 1:05:31
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr on Linux RHEL 9

• Commit ID: c257113
• Duration 1:09:10
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

• Commit ID: c257113
• Duration 2:05:47
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)
• Cluster Test Logs zip file of the test logs (available for 30 days)

[gxglass]

Looks fine to me. Claude has some possibly overthought suggestions which I think you can pursue at your discretion.

PR #12927 Analysis

Goal

This PR eliminates the IKnobCollection class hierarchy — an abstract interface with three concrete implementations (ClientKnobCollection, ServerKnobCollection, TestKnobCollection) — and replaces it with simple free functions and direct global pointer variables.

Before: CLIENT_KNOBS was a macro that called IKnobCollection::getGlobalKnobCollection().getClientKnobs() — a virtual dispatch through a unique_ptr singleton. The hierarchy existed so that client-only processes wouldn't accidentally access server knobs (they'd get
internal_error()), and to support a TestKnobCollection type.

After: CLIENT_KNOBS is a plain extern pointer (ClientKnobs const* CLIENT_KNOBS) pointing directly to a file-scope global. SERVER_KNOBS gets the same treatment. Free functions like resetClientKnobs(), setupClientKnobs(), setClientKnob() replace the virtual methods.

The hierarchy (5 files deleted: IKnobCollection.h/.cpp, ClientKnobCollection.h/.cpp, ServerKnobCollection.h/.cpp, TestKnobCollection.h/.cpp) is replaced by:

• New code in ClientKnobs.cpp for client-level knobs
• ServerKnobs.cpp moved from fdbclient/ to fdbserver/core/ (where it logically belongs) with parallel server-level free functions

Cleanliness Assessment

Positive:

• Removes unnecessary abstraction — the virtual dispatch was never needed at runtime; the type was always known statically at each call site
• TestKnobCollection was dead code (Type::TEST was never used outside IKnobCollection.cpp itself)
• Moving ServerKnobs.cpp from fdbclient/ to fdbserver/core/ is a good layering improvement
• Consistent pattern: each level (client, server) has a symmetric set of free functions
• The macro-to-extern-pointer change eliminates a virtual dispatch on every CLIENT_KNOBS-> / SERVER_KNOBS-> access, which happens frequently in hot paths

Minor style issues:

• #include is added to ClientKnobs.cpp and ServerKnobs.cpp — presumably for std::cerr in setupClientKnobs() / setupServerKnobs(). This was already used in the old code (via IKnobCollection::setupKnobs), so it's fine, but is a heavy include. The old code had
  it in IKnobCollection.cpp so this is just moved, not new.

Potential Issues

1. Duplicate global FlowKnobs/ClientKnobs instances — ODR / state divergence risk (Medium)

Both ClientKnobs.cpp and ServerKnobs.cpp define their own file-scope (namespace {}) global instances:

• ClientKnobs.cpp: FlowKnobs globalFlowKnobs; ClientKnobs globalClientKnobs; ClientKnobs bootstrapGlobalClientKnobs;
• ServerKnobs.cpp: FlowKnobs globalFlowKnobs; ClientKnobs globalClientKnobs; ServerKnobs globalServerKnobs; ClientKnobs bootstrapGlobalClientKnobs; ...

Since these are in anonymous namespaces, they are distinct objects. In a server process, resetServerKnobs() sets FLOW_KNOBS and CLIENT_KNOBS to point to the instances in ServerKnobs.cpp's anonymous namespace, while the instances in ClientKnobs.cpp go unused. This works but is
fragile — if someone calls resetClientKnobs() after resetServerKnobs() (or vice versa), the global pointers would silently switch to pointing at a different set of objects. The old code had a single setGlobalKnobCollection() with an ASSERT(FLOW_KNOBS == &bootstrapGlobalFlowKnobs)
that prevented double-initialization. That assertion is gone.

2. Lost bootstrap-to-global transition guard (Medium)

The old setGlobalKnobCollection() had ASSERT(FLOW_KNOBS == &bootstrapGlobalFlowKnobs) ensuring the global knob collection is only initialized once. Neither resetClientKnobs() nor resetServerKnobs() has any equivalent guard. Calling reset multiple times would silently
re-initialize all knobs, potentially discarding runtime modifications.

3. CLIENT_KNOBS initial value points to bootstrap, not null (Low - by design)

ClientKnobs.cpp initializes: ClientKnobs const* CLIENT_KNOBS = &bootstrapGlobalClientKnobs;
ServerKnobs.cpp initializes: ServerKnobs const* SERVER_KNOBS = &bootstrapGlobalServerKnobs;

This mirrors the existing FLOW_KNOBS bootstrap pattern. It's correct — code that runs before resetClientKnobs()/resetServerKnobs() gets sane defaults.

4. TestKnobs entirely removed — test knob infrastructure is gone (Low)

TestKnobCollection and TestKnobs (with TEST_LONG, TEST_INT, etc.) are deleted. There's no replacement. This is likely fine because Type::TEST was never actually used in any call to setGlobalKnobCollection() — it was dead code. But if any downstream/out-of-tree code depends on
TestKnobs or clearTestKnobs(), it will break.

5. const_cast pattern for mutability (Low - pre-existing)

The mutableFlowKnobs() / mutableClientKnobs() / mutableServerKnobs() helpers use const_cast to strip const from the global pointers. This is technically safe because the underlying objects are non-const, but it's a code smell. The old code also mutated through
getMutableGlobalKnobCollection(), so this isn't a regression.

6. No CMakeLists.txt changes shown for file moves/deletes

The diff moves ServerKnobs.cpp from fdbclient/ to fdbserver/core/ and deletes several .cpp files. The build system uses fdb_find_sources() which auto-discovers sources, so this likely works automatically. But if fdb_find_sources doesn't scan subdirectories or if there are
explicit source lists, the build could break. Worth verifying.

Summary

┌───────────────────────────────────────────────────────────────────────────┬───────────────────────────────────────────┐
│ Issue │ Severity │
├───────────────────────────────────────────────────────────────────────────┼───────────────────────────────────────────┤
│ Duplicate anonymous-namespace globals (FlowKnobs, ClientKnobs) in two TUs │ Medium — works but fragile │
├───────────────────────────────────────────────────────────────────────────┼───────────────────────────────────────────┤
│ Lost bootstrap assertion guard against double-init │ Medium — silent misconfiguration possible │
├───────────────────────────────────────────────────────────────────────────┼───────────────────────────────────────────┤
│ TestKnobs entirely removed (dead code) │ Low │
├───────────────────────────────────────────────────────────────────────────┼───────────────────────────────────────────┤
│ const_cast for mutation │ Low (pre-existing pattern) │
├───────────────────────────────────────────────────────────────────────────┼───────────────────────────────────────────┤
│ CMake auto-discovery of moved/deleted files │ Low (needs build verification) │
└───────────────────────────────────────────────────────────────────────────┴───────────────────────────────────────────┘

Overall this is a solid simplification. The main architectural concern is the duplicated global state across two translation units and the lack of a guard against calling both resetClientKnobs() and resetServerKnobs() or calling either one twice. A simple ASSERT or a flag could
prevent accidental misuse.

[foundationdb-ci]

Result of foundationdb-pr-clang-ide on Linux RHEL 9

• Commit ID: a5d8c90
• Duration 0:28:05
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

• Commit ID: a5d8c90
• Duration 0:33:44
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang-arm on Linux CentOS 7

• Commit ID: a5d8c90
• Duration 0:43:49
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-macos on macOS Ventura 13.x

• Commit ID: a5d8c90
• Duration 0:48:12
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang on Linux RHEL 9

• Commit ID: a5d8c90
• Duration 1:00:41
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr on Linux RHEL 9

• Commit ID: a5d8c90
• Duration 1:05:12
• Result: ❌ FAILED
• Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang-ide on Linux RHEL 9

• Commit ID: c095080
• Duration 0:23:00
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

• Commit ID: c095080
• Duration 0:35:13
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

• Commit ID: a5d8c90
• Duration 2:04:31
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)
• Cluster Test Logs zip file of the test logs (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang-arm on Linux CentOS 7

• Commit ID: c095080
• Duration 0:44:15
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang on Linux RHEL 9

• Commit ID: c095080
• Duration 0:48:45
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-macos on macOS Ventura 13.x

• Commit ID: c095080
• Duration 0:48:42
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr on Linux RHEL 9

• Commit ID: c095080
• Duration 0:59:29
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

• Commit ID: c095080
• Duration 2:11:52
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)
• Cluster Test Logs zip file of the test logs (available for 30 days)