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

@@ -4,6 +4,7 @@
 #include "src/server/config_registry.h"
 
 #include <absl/flags/reflection.h>
+#include <absl/strings/str_replace.h>
 
 #include "base/logging.h"
 
@@ -12,22 +13,29 @@ 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)
     return SetResult::READONLY;
 
   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<std::string> ConfigRegistry::Get(std::string_view config_name) {
+optional<string> 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<string> ConfigRegistry::List(string_view glob) const {
+  string normalized_glob = NormalizeConfigName(glob);
+
   vector<string> 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;
 

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.

src/server/server_family.cc

@@ -823,6 +823,7 @@ void ServerFamily::Init(util::AcceptServer* acceptor, std::vector<facade::Listen
   config_registry.RegisterMutable("tls_key_file");
   config_registry.RegisterMutable("tls_ca_cert_file");
   config_registry.RegisterMutable("tls_ca_cert_dir");
+  config_registry.RegisterMutable("replica_priority");
 
   pb_task_ = shard_set->pool()->GetNextProactor();
   if (pb_task_->GetKind() == ProactorBase::EPOLL) {
@@ -1765,9 +1766,12 @@ void ServerFamily::Config(CmdArgList args, ConnectionContext* cntx) {
       vector<string> 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<RedisReplyBuilder*>(cntx->reply_builder());

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