From f68fd28d73ccde1f186e2ed8ab7946b61b2528c8 Mon Sep 17 00:00:00 2001 From: Jingyu Zhou Date: Thu, 24 Mar 2022 15:17:02 -0700 Subject: [PATCH] Refactor duplicated code into IKnobCollection::setupKnobs() --- fdbbackup/FileDecoder.actor.cpp | 25 ++------------------- fdbbackup/backup.actor.cpp | 28 ++--------------------- fdbcli/fdbcli.actor.cpp | 27 ++--------------------- fdbclient/IKnobCollection.cpp | 25 +++++++++++++++++++++ fdbclient/IKnobCollection.h | 5 +++++ fdbserver/fdbserver.actor.cpp | 39 ++++++--------------------------- 6 files changed, 43 insertions(+), 106 deletions(-) diff --git a/fdbbackup/FileDecoder.actor.cpp b/fdbbackup/FileDecoder.actor.cpp index 7d9e27dcb1..c2fc6d1344 100644 --- a/fdbbackup/FileDecoder.actor.cpp +++ b/fdbbackup/FileDecoder.actor.cpp @@ -150,31 +150,10 @@ struct DecodeParams { } void updateKnobs() { - auto& g_knobs = IKnobCollection::getMutableGlobalKnobCollection(); - for (const auto& [knobName, knobValueString] : knobs) { - try { - auto knobValue = g_knobs.parseKnobValue(knobName, knobValueString); - g_knobs.setKnob(knobName, knobValue); - } catch (Error& e) { - if (e.code() == error_code_invalid_option_value) { - std::cerr << "WARNING: Invalid value '" << knobValueString << "' for knob option '" << knobName - << "'\n"; - TraceEvent(SevWarnAlways, "InvalidKnobValue") - .detail("Knob", printable(knobName)) - .detail("Value", printable(knobValueString)); - } else { - std::cerr << "ERROR: Failed to set knob option '" << knobName << "': " << e.what() << "\n"; - TraceEvent(SevError, "FailedToSetKnob") - .errorUnsuppressed(e) - .detail("Knob", printable(knobName)) - .detail("Value", printable(knobValueString)); - throw; - } - } - } + IKnobCollection::setupKnobs(knobs); // Reinitialize knobs in order to update knobs that are dependent on explicitly set knobs - g_knobs.initialize(Randomize::True, IsSimulated::False); + IKnobCollection::getMutableGlobalKnobCollection().initialize(Randomize::False, IsSimulated::False); } }; diff --git a/fdbbackup/backup.actor.cpp b/fdbbackup/backup.actor.cpp index a8b4218569..8cbe2102c6 100644 --- a/fdbbackup/backup.actor.cpp +++ b/fdbbackup/backup.actor.cpp @@ -3902,33 +3902,9 @@ int main(int argc, char* argv[]) { return FDB_EXIT_ERROR; } - auto& g_knobs = IKnobCollection::getMutableGlobalKnobCollection(); - for (const auto& [knobName, knobValueString] : knobs) { - try { - auto knobValue = g_knobs.parseKnobValue(knobName, knobValueString); - g_knobs.setKnob(knobName, knobValue); - } catch (Error& e) { - if (e.code() == error_code_invalid_option_value) { - fprintf(stderr, - "WARNING: Invalid value '%s' for knob option '%s'\n", - knobValueString.c_str(), - knobName.c_str()); - TraceEvent(SevWarnAlways, "InvalidKnobValue") - .detail("Knob", printable(knobName)) - .detail("Value", printable(knobValueString)); - } else { - fprintf(stderr, "ERROR: Failed to set knob option '%s': %s\n", knobName.c_str(), e.what()); - TraceEvent(SevError, "FailedToSetKnob") - .error(e) - .detail("Knob", printable(knobName)) - .detail("Value", printable(knobValueString)); - throw; - } - } - } - + IKnobCollection::setupKnobs(knobs); // Reinitialize knobs in order to update knobs that are dependent on explicitly set knobs - g_knobs.initialize(Randomize::False, IsSimulated::False); + IKnobCollection::getMutableGlobalKnobCollection().initialize(Randomize::False, IsSimulated::False); TraceEvent("ProgramStart") .setMaxEventLength(12000) diff --git a/fdbcli/fdbcli.actor.cpp b/fdbcli/fdbcli.actor.cpp index b89ecaab2a..55b70cbb5c 100644 --- a/fdbcli/fdbcli.actor.cpp +++ b/fdbcli/fdbcli.actor.cpp @@ -1021,33 +1021,10 @@ struct CLIOptions { } void setupKnobs() { - auto& g_knobs = IKnobCollection::getMutableGlobalKnobCollection(); - for (const auto& [knobName, knobValueString] : knobs) { - try { - auto knobValue = g_knobs.parseKnobValue(knobName, knobValueString); - g_knobs.setKnob(knobName, knobValue); - } catch (Error& e) { - if (e.code() == error_code_invalid_option_value) { - fprintf(stderr, - "WARNING: Invalid value '%s' for knob option '%s'\n", - knobValueString.c_str(), - knobName.c_str()); - TraceEvent(SevWarnAlways, "InvalidKnobValue") - .detail("Knob", printable(knobName)) - .detail("Value", printable(knobValueString)); - } else { - fprintf(stderr, "ERROR: Failed to set knob option '%s': %s\n", knobName.c_str(), e.what()); - TraceEvent(SevError, "FailedToSetKnob") - .error(e) - .detail("Knob", printable(knobName)) - .detail("Value", printable(knobValueString)); - exit_code = FDB_EXIT_ERROR; - } - } - } + IKnobCollection::setupKnobs(knobs); // Reinitialize knobs in order to update knobs that are dependent on explicitly set knobs - g_knobs.initialize(Randomize::False, IsSimulated::False); + IKnobCollection::getMutableGlobalKnobCollection().initialize(Randomize::False, IsSimulated::False); } int processArg(CSimpleOpt& args) { diff --git a/fdbclient/IKnobCollection.cpp b/fdbclient/IKnobCollection.cpp index 41d0441904..a170c8be80 100644 --- a/fdbclient/IKnobCollection.cpp +++ b/fdbclient/IKnobCollection.cpp @@ -95,6 +95,31 @@ IKnobCollection& IKnobCollection::getMutableGlobalKnobCollection() { return *globalKnobCollection(); } +void IKnobCollection::setupKnobs(const std::vector>& knobs) { + auto& g_knobs = IKnobCollection::getMutableGlobalKnobCollection(); + for (const auto& [knobName, knobValueString] : knobs) { + try { + auto knobValue = g_knobs.parseKnobValue(knobName, knobValueString); + g_knobs.setKnob(knobName, knobValue); + } catch (Error& e) { + if (e.code() == error_code_invalid_option_value) { + std::cerr << "WARNING: Invalid value '" << knobValueString << "' for knob option '" << knobName + << "'\n"; + TraceEvent(SevWarnAlways, "InvalidKnobValue") + .detail("Knob", printable(knobName)) + .detail("Value", printable(knobValueString)); + } else { + std::cerr << "ERROR: Failed to set knob option '" << knobName << "': " << e.what() << "\n"; + TraceEvent(SevError, "FailedToSetKnob") + .errorUnsuppressed(e) + .detail("Knob", printable(knobName)) + .detail("Value", printable(knobValueString)); + throw e; + } + } + } +} + ConfigMutationRef IKnobCollection::createSetMutation(Arena arena, KeyRef key, ValueRef value) { ConfigKey configKey = ConfigKeyRef::decodeKey(key); auto knobValue = diff --git a/fdbclient/IKnobCollection.h b/fdbclient/IKnobCollection.h index d37955ce55..eff02420f4 100644 --- a/fdbclient/IKnobCollection.h +++ b/fdbclient/IKnobCollection.h @@ -69,6 +69,11 @@ public: static void setGlobalKnobCollection(Type, Randomize, IsSimulated); static IKnobCollection const& getGlobalKnobCollection(); static IKnobCollection& getMutableGlobalKnobCollection(); + + // Sets up a list of pairs. If encounter a failure, + // immediately throws the error. + static void setupKnobs(const std::vector>& knobs); + static ConfigMutationRef createSetMutation(Arena, KeyRef, ValueRef); static ConfigMutationRef createClearMutation(Arena, KeyRef); }; diff --git a/fdbserver/fdbserver.actor.cpp b/fdbserver/fdbserver.actor.cpp index 556f16728b..7803b084df 100644 --- a/fdbserver/fdbserver.actor.cpp +++ b/fdbserver/fdbserver.actor.cpp @@ -1780,42 +1780,17 @@ int main(int argc, char* argv[]) { Randomize::True, role == ServerRole::Simulation ? IsSimulated::True : IsSimulated::False); - IKnobCollection::getMutableGlobalKnobCollection().setKnob("log_directory", KnobValue::create(opts.logFolder)); - IKnobCollection::getMutableGlobalKnobCollection().setKnob("conn_file", KnobValue::create(opts.connFile)); + auto& g_knobs = IKnobCollection::getMutableGlobalKnobCollection(); + g_knobs.setKnob("log_directory", KnobValue::create(opts.logFolder)); + g_knobs.setKnob("conn_file", KnobValue::create(opts.connFile)); if (role != ServerRole::Simulation) { - IKnobCollection::getMutableGlobalKnobCollection().setKnob("commit_batches_mem_bytes_hard_limit", - KnobValue::create(int64_t{ opts.memLimit })); + g_knobs.setKnob("commit_batches_mem_bytes_hard_limit", KnobValue::create(int64_t{ opts.memLimit })); } - for (const auto& [knobName, knobValueString] : opts.knobs) { - try { - auto& g_knobs = IKnobCollection::getMutableGlobalKnobCollection(); - auto knobValue = g_knobs.parseKnobValue(knobName, knobValueString); - g_knobs.setKnob(knobName, knobValue); - } catch (Error& e) { - if (e.code() == error_code_invalid_option_value) { - fprintf(stderr, - "WARNING: Invalid value '%s' for knob option '%s'\n", - knobName.c_str(), - knobValueString.c_str()); - TraceEvent(SevWarnAlways, "InvalidKnobValue") - .detail("Knob", printable(knobName)) - .detail("Value", printable(knobValueString)); - } else { - fprintf(stderr, "ERROR: Failed to set knob option '%s': %s\n", knobName.c_str(), e.what()); - TraceEvent(SevError, "FailedToSetKnob") - .error(e) - .detail("Knob", printable(knobName)) - .detail("Value", printable(knobValueString)); - throw; - } - } - } - IKnobCollection::getMutableGlobalKnobCollection().setKnob("server_mem_limit", - KnobValue::create(int64_t{ opts.memLimit })); + IKnobCollection::setupKnobs(opts.knobs); + g_knobs.setKnob("server_mem_limit", KnobValue::create(int64_t{ opts.memLimit })); // Reinitialize knobs in order to update knobs that are dependent on explicitly set knobs - IKnobCollection::getMutableGlobalKnobCollection().initialize( - Randomize::True, role == ServerRole::Simulation ? IsSimulated::True : IsSimulated::False); + g_knobs.initialize(Randomize::True, role == ServerRole::Simulation ? IsSimulated::True : IsSimulated::False); // evictionPolicyStringToEnum will throw an exception if the string is not recognized as a valid EvictablePageCache::evictionPolicyStringToEnum(FLOW_KNOBS->CACHE_EVICTION_POLICY);