From 90abd98f3a3cc80c96ef16a892b5c5eb2af966ac Mon Sep 17 00:00:00 2001 From: Syed Paymaan Raza <1238752+spraza@users.noreply.github.com> Date: Fri, 3 Oct 2025 11:45:41 -0700 Subject: [PATCH] 5s: fuzz write knob --- fdbclient/ClientKnobCollection.cpp | 6 +++--- fdbclient/ClientKnobs.cpp | 23 +++++++++++++++++------ fdbclient/ServerKnobCollection.cpp | 11 ++++++++++- fdbclient/ServerKnobs.cpp | 16 ++++------------ fdbclient/include/fdbclient/ClientKnobs.h | 4 ++-- tests/fast/AutomaticIdempotency.toml | 4 ++++ 6 files changed, 40 insertions(+), 24 deletions(-) diff --git a/fdbclient/ClientKnobCollection.cpp b/fdbclient/ClientKnobCollection.cpp index 0fa3a4d69b8..f6b499a10e4 100644 --- a/fdbclient/ClientKnobCollection.cpp +++ b/fdbclient/ClientKnobCollection.cpp @@ -21,16 +21,16 @@ #include "fdbclient/ClientKnobCollection.h" ClientKnobCollection::ClientKnobCollection(Randomize randomize, IsSimulated isSimulated) - : flowKnobs(randomize, isSimulated), clientKnobs(randomize) {} + : flowKnobs(randomize, isSimulated), clientKnobs(randomize, isSimulated) {} void ClientKnobCollection::initialize(Randomize randomize, IsSimulated isSimulated) { flowKnobs.initialize(randomize, isSimulated); - clientKnobs.initialize(randomize); + clientKnobs.initialize(randomize, isSimulated); } void ClientKnobCollection::reset(Randomize randomize, IsSimulated isSimulated) { flowKnobs.reset(randomize, isSimulated); - clientKnobs.reset(randomize); + clientKnobs.reset(randomize, isSimulated); } Optional ClientKnobCollection::tryParseKnobValue(std::string const& knobName, diff --git a/fdbclient/ClientKnobs.cpp b/fdbclient/ClientKnobs.cpp index 6957c0eb446..f360637a90e 100644 --- a/fdbclient/ClientKnobs.cpp +++ b/fdbclient/ClientKnobs.cpp @@ -23,16 +23,27 @@ #include "fdbclient/SystemData.h" #include "fdbclient/Tenant.h" #include "flow/IRandom.h" +#include "flow/Knobs.h" #include "flow/UnitTest.h" #include "flow/flow.h" +// Returns a deterministically random transaction timeout value for simulation testing. +// More weight is given to the famous 5s timeout, but [1, 10] range is returned with lower weight. +int randomTxnTimeoutSeconds() { + if (deterministicRandom()->truePercent(90)) { + return 5; + } else { + return deterministicRandom()->randomInt(1, 11); // [1, 10] + } +} + #define init(...) KNOB_FN(__VA_ARGS__, INIT_ATOMIC_KNOB, INIT_KNOB)(__VA_ARGS__) -ClientKnobs::ClientKnobs(Randomize randomize) { - initialize(randomize); +ClientKnobs::ClientKnobs(Randomize randomize, IsSimulated isSimulated) { + initialize(randomize, isSimulated); } -void ClientKnobs::initialize(Randomize randomize) { +void ClientKnobs::initialize(Randomize randomize, IsSimulated isSimulated) { // clang-format off init( TOO_MANY, 1000000 ); @@ -126,7 +137,7 @@ void ClientKnobs::initialize(Randomize randomize) { // Versions -- knobs that control 5s timeout init( VERSIONS_PER_SECOND, 1e6 ); // Must be the same as SERVER_KNOBS->VERSIONS_PER_SECOND - init( MAX_WRITE_TRANSACTION_LIFE_VERSIONS, 5 * VERSIONS_PER_SECOND); // Must be the same as SERVER_KNOBS->MAX_WRITE_TRANSACTION_LIFE_VERSIONS + init( MAX_WRITE_TRANSACTION_LIFE_VERSIONS, 5 * VERSIONS_PER_SECOND); if (isSimulated) MAX_WRITE_TRANSACTION_LIFE_VERSIONS = randomTxnTimeoutSeconds() * VERSIONS_PER_SECOND; // Core init( CORE_VERSIONSPERSECOND, 1e6 ); @@ -354,13 +365,13 @@ void ClientKnobs::initialize(Randomize randomize) { TEST_CASE("/fdbclient/knobs/initialize") { // This test depends on TASKBUCKET_TIMEOUT_VERSIONS being defined as a constant multiple of CORE_VERSIONSPERSECOND - ClientKnobs clientKnobs(Randomize::False); + ClientKnobs clientKnobs(Randomize::False, IsSimulated::False); int64_t initialCoreVersionsPerSecond = clientKnobs.CORE_VERSIONSPERSECOND; int initialTaskBucketTimeoutVersions = clientKnobs.TASKBUCKET_TIMEOUT_VERSIONS; clientKnobs.setKnob("core_versionspersecond", initialCoreVersionsPerSecond * 2); ASSERT_EQ(clientKnobs.CORE_VERSIONSPERSECOND, initialCoreVersionsPerSecond * 2); ASSERT_EQ(clientKnobs.TASKBUCKET_TIMEOUT_VERSIONS, initialTaskBucketTimeoutVersions); - clientKnobs.initialize(Randomize::False); + clientKnobs.initialize(Randomize::False, IsSimulated::False); ASSERT_EQ(clientKnobs.CORE_VERSIONSPERSECOND, initialCoreVersionsPerSecond * 2); ASSERT_EQ(clientKnobs.TASKBUCKET_TIMEOUT_VERSIONS, initialTaskBucketTimeoutVersions * 2); return Void(); diff --git a/fdbclient/ServerKnobCollection.cpp b/fdbclient/ServerKnobCollection.cpp index 99413ceb238..3702f177250 100644 --- a/fdbclient/ServerKnobCollection.cpp +++ b/fdbclient/ServerKnobCollection.cpp @@ -48,7 +48,16 @@ Optional ServerKnobCollection::tryParseKnobValue(std::string const& k } bool ServerKnobCollection::trySetKnob(std::string const& knobName, KnobValueRef const& knobValue) { - return clientKnobCollection.trySetKnob(knobName, knobValue) || knobValue.visitSetKnob(knobName, serverKnobs); + // Do not short circuit by directly returning: + // clientKnobCollection.trySetKnob(knobName, knobValue) || knobValue.visitSetKnob(knobName, serverKnobs) + // This is because some knobs have the same name in client and server e.g. MAX_WRITE_TRANSACTION_LIFE_VERSIONS + // When setting such knobs, we want both client and server knob to have its value updated + // Short circuiting would mean that server knob FOO won't be updated if client knob FOO was updated + // Instead, we attempt setting client and server knobs in separate statements, and return true + // if at least one of the set attempts was succesful. + const bool setClientKnob = clientKnobCollection.trySetKnob(knobName, knobValue); + const bool setServerKnob = knobValue.visitSetKnob(knobName, serverKnobs); + return setClientKnob || setServerKnob; } bool ServerKnobCollection::isAtomic(std::string const& knobName) const { diff --git a/fdbclient/ServerKnobs.cpp b/fdbclient/ServerKnobs.cpp index 82c33ddbde3..57a112179cc 100644 --- a/fdbclient/ServerKnobs.cpp +++ b/fdbclient/ServerKnobs.cpp @@ -19,26 +19,19 @@ */ #include "fdbclient/ServerKnobs.h" +#include "fdbclient/IKnobCollection.h" #include "flow/CompressionUtils.h" #include "flow/IRandom.h" #include "flow/flow.h" +extern int randomTxnTimeoutSeconds(); + #define init(...) KNOB_FN(__VA_ARGS__, INIT_ATOMIC_KNOB, INIT_KNOB)(__VA_ARGS__) ServerKnobs::ServerKnobs(Randomize randomize, ClientKnobs* clientKnobs, IsSimulated isSimulated) { initialize(randomize, clientKnobs, isSimulated); } -// Returns a deterministically random transaction timeout value for simulation testing. -// More weight is given to the famous 5s timeout, but [1, 10] range is returned with lower weight. -int randomTxnTimeoutSeconds() { - if (deterministicRandom()->truePercent(90)) { - return 5; - } else { - return deterministicRandom()->randomInt(1, 11); // [1, 10] - } -} - void ServerKnobs::initialize(Randomize randomize, ClientKnobs* clientKnobs, IsSimulated isSimulated) { // clang-format off init( ALLOW_DANGEROUS_KNOBS, isSimulated ); @@ -46,7 +39,7 @@ void ServerKnobs::initialize(Randomize randomize, ClientKnobs* clientKnobs, IsSi // Versions -- knobs that control 5s timeout init( VERSIONS_PER_SECOND, 1e6 ); init( MAX_READ_TRANSACTION_LIFE_VERSIONS, 5 * VERSIONS_PER_SECOND ); if (isSimulated) MAX_READ_TRANSACTION_LIFE_VERSIONS = randomTxnTimeoutSeconds() * VERSIONS_PER_SECOND; - init( MAX_WRITE_TRANSACTION_LIFE_VERSIONS, 5 * VERSIONS_PER_SECOND ); if (randomize && BUGGIFY) MAX_WRITE_TRANSACTION_LIFE_VERSIONS=std::max(1, 1 * VERSIONS_PER_SECOND); + init( MAX_WRITE_TRANSACTION_LIFE_VERSIONS, 5 * VERSIONS_PER_SECOND ); if (isSimulated) MAX_WRITE_TRANSACTION_LIFE_VERSIONS = clientKnobs->MAX_WRITE_TRANSACTION_LIFE_VERSIONS; // Versions -- other init( MAX_VERSIONS_IN_FLIGHT, 100 * VERSIONS_PER_SECOND ); @@ -56,7 +49,6 @@ void ServerKnobs::initialize(Randomize randomize, ClientKnobs* clientKnobs, IsSi init( ENABLE_VERSION_VECTOR_TLOG_UNICAST, false ); init( ENABLE_VERSION_VECTOR_HA_OPTIMIZATION, false ); init( ENABLE_VERSION_VECTOR_REPLY_RECOVERY, false ); - init( MAX_COMMIT_BATCH_INTERVAL, 2.0 ); if( randomize && BUGGIFY ) MAX_COMMIT_BATCH_INTERVAL = 0.5; // Each commit proxy generates a CommitTransactionBatchRequest at least this often, so that versions always advance smoothly MAX_COMMIT_BATCH_INTERVAL = std::min(MAX_COMMIT_BATCH_INTERVAL, MAX_READ_TRANSACTION_LIFE_VERSIONS/double(2*VERSIONS_PER_SECOND)); // Ensure that the proxy commits 2 times every MAX_READ_TRANSACTION_LIFE_VERSIONS, otherwise the master will not give out versions fast enough MAX_COMMIT_BATCH_INTERVAL = std::min(MAX_COMMIT_BATCH_INTERVAL, MAX_WRITE_TRANSACTION_LIFE_VERSIONS/double(2*VERSIONS_PER_SECOND)); // Ensure that the proxy commits 2 times every MAX_WRITE_TRANSACTION_LIFE_VERSIONS, otherwise the master will not give out versions fast enough diff --git a/fdbclient/include/fdbclient/ClientKnobs.h b/fdbclient/include/fdbclient/ClientKnobs.h index e174d28484f..886ce5732d3 100644 --- a/fdbclient/include/fdbclient/ClientKnobs.h +++ b/fdbclient/include/fdbclient/ClientKnobs.h @@ -365,8 +365,8 @@ class SWIFT_CXX_IMMORTAL_SINGLETON_TYPE ClientKnobs : public KnobsImpl