From 89a48a7aa810908d18709976231857ce870e8224 Mon Sep 17 00:00:00 2001 From: Shahar Mike Date: Mon, 29 Jul 2024 23:02:49 +0300 Subject: [PATCH] chore: Support setting the value of `replica-priority` (#3400) * chore: Support setting the value of `replica-priority` This PR adds a small refactor to the way we set and get config names which have dashes (`-`) and underscores (`_`). Until now, words were separated by underscores because this is how our flags library (absl) works. However, this is incompatible with Valkey, which uses dashes as a word separator. Once merged, we will support both underscores and dashes in config names, but will only return the name with dashes. **This is a behavior change**. We're doing this in order to be compatible with `replica-priority` and possibly other config names that Valkey uses. * Flag restore * normalize to '_' --- src/server/config_registry.cc | 36 ++++++++++++++++++++++---------- src/server/config_registry.h | 2 ++ src/server/server_family.cc | 10 ++++++--- src/server/server_family_test.cc | 29 +++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 14 deletions(-) diff --git a/src/server/config_registry.cc b/src/server/config_registry.cc index 5a8041589f58..9206b4772ff2 100644 --- a/src/server/config_registry.cc +++ b/src/server/config_registry.cc @@ -4,6 +4,7 @@ #include "src/server/config_registry.h" #include +#include #include "base/logging.h" @@ -12,13 +13,20 @@ extern "C" { } namespace dfly { - +namespace { using namespace std; +string NormalizeConfigName(string_view name) { + return absl::StrReplaceAll(name, {{"-", "_"}}); +} +} // namespace + // Returns true if the value was updated. -auto ConfigRegistry::Set(std::string_view config_name, std::string_view value) -> SetResult { +auto ConfigRegistry::Set(string_view config_name, string_view value) -> SetResult { + string name = NormalizeConfigName(config_name); + unique_lock lk(mu_); - auto it = registry_.find(config_name); + auto it = registry_.find(name); if (it == registry_.end()) return SetResult::UNKNOWN; if (!it->second.is_mutable) @@ -26,8 +34,8 @@ auto ConfigRegistry::Set(std::string_view config_name, std::string_view value) - auto cb = it->second.cb; - absl::CommandLineFlag* flag = absl::FindCommandLineFlag(config_name); - CHECK(flag); + absl::CommandLineFlag* flag = absl::FindCommandLineFlag(name); + CHECK(flag) << config_name; if (string error; !flag->ParseFrom(value, &error)) { LOG(WARNING) << error; return SetResult::INVALID; @@ -37,13 +45,15 @@ auto ConfigRegistry::Set(std::string_view config_name, std::string_view value) - return success ? SetResult::OK : SetResult::INVALID; } -std::optional ConfigRegistry::Get(std::string_view config_name) { +optional ConfigRegistry::Get(string_view config_name) { + string name = NormalizeConfigName(config_name); + unique_lock lk(mu_); - if (!registry_.contains(config_name)) - return std::nullopt; + if (!registry_.contains(name)) + return nullopt; lk.unlock(); - absl::CommandLineFlag* flag = absl::FindCommandLineFlag(config_name); + absl::CommandLineFlag* flag = absl::FindCommandLineFlag(name); CHECK(flag); return flag->CurrentValue(); } @@ -54,16 +64,20 @@ void ConfigRegistry::Reset() { } vector ConfigRegistry::List(string_view glob) const { + string normalized_glob = NormalizeConfigName(glob); + vector res; unique_lock lk(mu_); for (const auto& [name, _] : registry_) { - if (stringmatchlen(glob.data(), glob.size(), name.data(), name.size(), 1)) + if (stringmatchlen(normalized_glob.data(), normalized_glob.size(), name.data(), name.size(), 1)) res.push_back(name); } return res; } -void ConfigRegistry::RegisterInternal(std::string_view name, bool is_mutable, WriteCb cb) { +void ConfigRegistry::RegisterInternal(string_view config_name, bool is_mutable, WriteCb cb) { + string name = NormalizeConfigName(config_name); + absl::CommandLineFlag* flag = absl::FindCommandLineFlag(name); CHECK(flag) << "Unknown config name: " << name; diff --git a/src/server/config_registry.h b/src/server/config_registry.h index d017bf72ffcd..7ab2ccc51612 100644 --- a/src/server/config_registry.h +++ b/src/server/config_registry.h @@ -9,6 +9,8 @@ namespace dfly { +// Allows reading and modifying pre-registered configuration values by string names. +// This class treats dashes (-) are as underscores (_). class ConfigRegistry { public: // Accepts the new value as argument. Return true if config was successfully updated. diff --git a/src/server/server_family.cc b/src/server/server_family.cc index 069cac6df166..4b6d8646ff33 100644 --- a/src/server/server_family.cc +++ b/src/server/server_family.cc @@ -823,6 +823,7 @@ void ServerFamily::Init(util::AcceptServer* acceptor, std::vectorpool()->GetNextProactor(); if (pb_task_->GetKind() == ProactorBase::EPOLL) { @@ -1765,9 +1766,12 @@ void ServerFamily::Config(CmdArgList args, ConnectionContext* cntx) { vector names = config_registry.List(param); for (const auto& name : names) { - absl::CommandLineFlag* flag = CHECK_NOTNULL(absl::FindCommandLineFlag(name)); - res.push_back(name); - res.push_back(flag->CurrentValue()); + auto value = config_registry.Get(name); + DCHECK(value.has_value()); + if (value.has_value()) { + res.push_back(name); + res.push_back(*value); + } } } auto* rb = static_cast(cntx->reply_builder()); diff --git a/src/server/server_family_test.cc b/src/server/server_family_test.cc index aca7f66b3c2d..abeb5317fb1c 100644 --- a/src/server/server_family_test.cc +++ b/src/server/server_family_test.cc @@ -516,4 +516,33 @@ TEST_F(ServerFamilyTest, ClientTrackingLuaBug) { EXPECT_EQ(InvalidationMessagesLen("IO0"), 3); } +TEST_F(ServerFamilyTest, ConfigNormalization) { + // TODO: Ideally we'd also test that INFO REPLICATION returns the value set in the config, but + // there is no way currently to setup a mock replica in unit tests. + + absl::FlagSaver fs; // Restores the flag to default value after test finishes + + // Default value + EXPECT_THAT(Run({"config", "get", "replica-priority"}), + RespArray(ElementsAre("replica_priority", "100"))); + EXPECT_THAT(Run({"config", "get", "replica_priority"}), + RespArray(ElementsAre("replica_priority", "100"))); + + // Set with dash + EXPECT_THAT(Run({"config", "set", "replica-priority", "7"}), "OK"); + + EXPECT_THAT(Run({"config", "get", "replica-priority"}), + RespArray(ElementsAre("replica_priority", "7"))); + EXPECT_THAT(Run({"config", "get", "replica_priority"}), + RespArray(ElementsAre("replica_priority", "7"))); + + // Set with underscore + EXPECT_THAT(Run({"config", "set", "replica_priority", "13"}), "OK"); + + EXPECT_THAT(Run({"config", "get", "replica-priority"}), + RespArray(ElementsAre("replica_priority", "13"))); + EXPECT_THAT(Run({"config", "get", "replica_priority"}), + RespArray(ElementsAre("replica_priority", "13"))); +} + } // namespace dfly