This is an automated email from the ASF dual-hosted git repository.

twice pushed a commit to branch unstable
in repository https://gitbox.apache.org/repos/asf/incubator-kvrocks.git


The following commit(s) were added to refs/heads/unstable by this push:
     new c09cb8c4 Support CLI options for kvrocks server (#1037)
c09cb8c4 is described below

commit c09cb8c4054efadd3958046ee5f92b88a477eabf
Author: Twice <[email protected]>
AuthorDate: Mon Oct 24 23:29:14 2022 +0800

    Support CLI options for kvrocks server (#1037)
    
    * Support CLI options for kvrocks server
    
    * read from stdin
    
    * fix usage
    
    * fix
    
    * remove header
---
 src/config/config.cc                               | 62 +++++++++++--------
 src/config/config.h                                | 11 +++-
 src/config/config_type.h                           |  8 +--
 src/main.cc                                        | 71 +++++++++++++---------
 tests/cppunit/config_test.cc                       | 18 +++---
 .../integration/cli_options/cli_options_test.go    | 39 ++++++++++++
 tests/gocase/util/server.go                        |  5 ++
 7 files changed, 147 insertions(+), 67 deletions(-)

diff --git a/src/config/config.cc b/src/config/config.cc
index 4cb82b7c..6dd24856 100644
--- a/src/config/config.cc
+++ b/src/config/config.cc
@@ -601,32 +601,36 @@ void Config::ClearMaster() {
   }
 }
 
-Status Config::parseConfigFromString(const std::string &input, int 
line_number) {
-  auto parsed = ParseConfigLine(input);
-  if (!parsed) return parsed.ToStatus();
-
-  auto kv = std::move(*parsed);
-
-  if (kv.first.empty() || kv.second.empty()) return Status::OK();
-
-  std::string field_key = Util::ToLower(kv.first);
+Status Config::parseConfigFromPair(const std::pair<std::string, std::string> 
&input, int line_number) {
+  std::string field_key = Util::ToLower(input.first);
   const char ns_str[] = "namespace.";
   size_t ns_str_size = sizeof(ns_str) - 1;
-  if (!strncasecmp(kv.first.data(), ns_str, ns_str_size)) {
+  if (!strncasecmp(input.first.data(), ns_str, ns_str_size)) {
     // namespace should keep key case-sensitive
-    field_key = kv.first;
-    tokens[kv.second] = kv.first.substr(ns_str_size);
+    field_key = input.first;
+    tokens[input.second] = input.first.substr(ns_str_size);
   }
   auto iter = fields_.find(field_key);
   if (iter != fields_.end()) {
     auto &field = iter->second;
     field->line_number = line_number;
-    auto s = field->Set(kv.second);
+    auto s = field->Set(input.second);
     if (!s.IsOK()) return s;
   }
   return Status::OK();
 }
 
+Status Config::parseConfigFromString(const std::string &input, int 
line_number) {
+  auto parsed = ParseConfigLine(input);
+  if (!parsed) return parsed.ToStatus();
+
+  auto kv = std::move(*parsed);
+
+  if (kv.first.empty() || kv.second.empty()) return Status::OK();
+
+  return parseConfigFromPair(kv, line_number);
+}
+
 Status Config::finish() {
   if (requirepass.empty() && !tokens.empty()) {
     return Status(Status::NotOK, "requirepass empty wasn't allowed while the 
namespace exists");
@@ -657,30 +661,40 @@ Status Config::finish() {
   return Status::OK();
 }
 
-Status Config::Load(const std::string &path) {
-  if (!path.empty()) {
-    path_ = path;
-    std::ifstream file(path_);
-    if (!file.is_open()) return Status(Status::NotOK, strerror(errno));
+Status Config::Load(const CLIOptions &opts) {
+  if (!opts.conf_file.empty()) {
+    std::ifstream file;
+    std::istream *in = nullptr;
+    if (opts.conf_file == "-") {
+      in = &std::cin;
+    } else {
+      path_ = opts.conf_file;
+      file.open(path_);
+      if (!file.is_open()) return Status(Status::NotOK, strerror(errno));
+      in = &file;
+    }
 
     std::string line;
     int line_num = 1;
-    while (!file.eof()) {
-      std::getline(file, line);
-      Status s = parseConfigFromString(line, line_num);
-      if (!s.IsOK()) {
-        file.close();
+    while (!in->eof()) {
+      std::getline(*in, line);
+      if (auto s = parseConfigFromString(line, line_num); !s) {
         return Status(Status::NotOK, "at line: #L" + std::to_string(line_num) 
+ ", err: " + s.Msg());
       }
       line_num++;
     }
-    file.close();
   } else {
     std::cout << "Warn: no config file specified, using the default config. "
                  "In order to specify a config file use kvrocks -c 
/path/to/kvrocks.conf"
               << std::endl;
   }
 
+  for (const auto &opt : opts.cli_options) {
+    if (Status s = parseConfigFromPair(opt, -1); !s) {
+      return Status(Status::NotOK, "CLI config option error: " + s.Msg());
+    }
+  }
+
   for (const auto &iter : fields_) {
     // line_number = 0 means the user didn't specify the field value
     // on config file and would use default value, so won't validate here.
diff --git a/src/config/config.h b/src/config/config.h
index 68c028b1..6e3556fd 100644
--- a/src/config/config.h
+++ b/src/config/config.h
@@ -61,6 +61,14 @@ struct CompactionCheckerRange {
   bool Enabled() { return Start != -1 || Stop != -1; }
 };
 
+struct CLIOptions {
+  std::string conf_file;
+  std::vector<std::pair<std::string, std::string>> cli_options;
+
+  CLIOptions() = default;
+  explicit CLIOptions(std::string_view file) : conf_file(file) {}
+};
+
 struct Config {
  public:
   Config();
@@ -186,7 +194,7 @@ struct Config {
 
  public:
   Status Rewrite();
-  Status Load(const std::string &path);
+  Status Load(const CLIOptions &path);
   void Get(std::string key, std::vector<std::string> *values);
   Status Set(Server *svr, std::string key, const std::string &value);
   void SetMaster(const std::string &host, int port);
@@ -209,6 +217,7 @@ struct Config {
 
   void initFieldValidator();
   void initFieldCallback();
+  Status parseConfigFromPair(const std::pair<std::string, std::string> &input, 
int line_number);
   Status parseConfigFromString(const std::string &input, int line_number);
   Status finish();
   Status isNamespaceLegal(const std::string &ns);
diff --git a/src/config/config_type.h b/src/config/config_type.h
index 2148cc06..998a3bc4 100644
--- a/src/config/config_type.h
+++ b/src/config/config_type.h
@@ -29,13 +29,13 @@
 // forward declaration
 class Server;
 
-typedef std::function<Status(const std::string &, const std::string &)> 
validate_fn;
-typedef std::function<Status(Server *srv, const std::string &, const 
std::string &)> callback_fn;
+using validate_fn = std::function<Status(const std::string &, const 
std::string &)>;
+using callback_fn = std::function<Status(Server *, const std::string &, const 
std::string &)>;
 
-typedef struct configEnum {
+struct configEnum {
   const char *name;
   const int val;
-} configEnum;
+};
 int configEnumGetValue(configEnum *ce, const char *name);
 const char *configEnumGetName(configEnum *ce, int val);
 
diff --git a/src/main.cc b/src/main.cc
index 4a28673e..9fa195c8 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -20,10 +20,12 @@
 
 #include <dlfcn.h>
 #include <fcntl.h>
-#include <getopt.h>
 #include <stdlib.h>
 #include <string.h>
 #include <sys/stat.h>
+
+#include <iomanip>
+#include <ostream>
 #ifdef __linux__
 #define _XOPEN_SOURCE 700
 #else
@@ -49,11 +51,6 @@ bool Symbolize(void *pc, char *out, size_t out_size);
 
 std::function<void()> hup_handler;
 
-struct Options {
-  std::string conf_file;
-  bool show_usage = false;
-};
-
 Server *srv = nullptr;
 
 Server *GetServer() { return srv; }
@@ -112,30 +109,47 @@ void setupSigSegvAction() {
   sigaction(SIGINT, &act, nullptr);
 }
 
-static void usage(const char *program) {
-  std::cout << program << " implements the Redis protocol based on rocksdb\n"
-            << "\t-c config file\n"
-            << "\t-h help\n";
-  exit(0);
+struct NewOpt {
+  friend auto &operator<<(std::ostream &os, NewOpt) { return os << 
std::string(4, ' ') << std::setw(32); }
+} new_opt;
+
+static void printUsage(const char *program) {
+  std::cout << program << " implements the Redis protocol based on rocksdb" << 
std::endl
+            << "Usage:" << std::endl
+            << std::left << new_opt << "-c, --config <filename>"
+            << "set config file to <filename>, or `-' for stdin" << std::endl
+            << new_opt << "-v, --version"
+            << "print version information" << std::endl
+            << new_opt << "-h, --help"
+            << "print this help message" << std::endl
+            << new_opt << "--<config-key> <config-value>"
+            << "overwrite specific config option <config-key> to 
<config-value>" << std::endl;
 }
 
-static Options parseCommandLineOptions(int argc, char **argv) {
-  int ch;
-  Options opts;
-  while ((ch = ::getopt(argc, argv, "c:hv")) != -1) {
-    switch (ch) {
-      case 'c':
-        opts.conf_file = optarg;
-        break;
-      case 'h':
-        opts.show_usage = true;
-        break;
-      case 'v':
-        exit(0);
-      default:
-        usage(argv[0]);
+static void printVersion(std::ostream &os) { os << "kvrocks version " << 
VERSION << " @" << GIT_COMMIT << std::endl; }
+
+static CLIOptions parseCommandLineOptions(int argc, char **argv) {
+  using namespace std::string_view_literals;
+  CLIOptions opts;
+
+  for (int i = 1; i < argc; ++i) {
+    if ((argv[i] == "-c"sv || argv[i] == "--config"sv) && i + 1 < argc) {
+      opts.conf_file = argv[++i];
+    } else if (argv[i] == "-v"sv || argv[i] == "--version"sv) {
+      printVersion(std::cout);
+      std::exit(0);
+    } else if (argv[i] == "-h"sv || argv[i] == "--help"sv) {
+      printUsage(*argv);
+      std::exit(0);
+    } else if (std::string_view(argv[i], 2) == "--" && 
std::string_view(argv[i]).size() > 2 && i + 1 < argc) {
+      auto key = std::string_view(argv[i] + 2);
+      opts.cli_options.emplace_back(key, argv[++i]);
+    } else {
+      printUsage(*argv);
+      std::exit(1);
     }
   }
+
   return opts;
 }
 
@@ -272,19 +286,18 @@ int main(int argc, char *argv[]) {
   setupSigSegvAction();
 
   auto opts = parseCommandLineOptions(argc, argv);
-  if (opts.show_usage) usage(argv[0]);
 
   Redis::InitCommandsTable();
   Redis::PopulateCommands();
 
   Config config;
-  Status s = config.Load(opts.conf_file);
+  Status s = config.Load(opts);
   if (!s.IsOK()) {
     std::cout << "Failed to load config, err: " << s.Msg() << std::endl;
     exit(1);
   }
   initGoogleLog(&config);
-  LOG(INFO) << "Version: " << VERSION << " @" << GIT_COMMIT << std::endl;
+  printVersion(LOG(INFO));
   // Tricky: We don't expect that different instances running on the same port,
   // but the server use REUSE_PORT to support the multi listeners. So we 
connect
   // the listen port to check if the port has already listened or not.
diff --git a/tests/cppunit/config_test.cc b/tests/cppunit/config_test.cc
index 4a9564b1..4d3812f9 100644
--- a/tests/cppunit/config_test.cc
+++ b/tests/cppunit/config_test.cc
@@ -34,7 +34,7 @@ TEST(Config, GetAndSet) {
   const char *path = "test.conf";
   Config config;
 
-  config.Load(path);
+  config.Load(CLIOptions(path));
   // Config.Set need accessing commands, so we should init and populate
   // the command table here.
   Redis::InitCommandsTable();
@@ -94,7 +94,7 @@ TEST(Config, GetAndSet) {
     EXPECT_EQ(values[1], iter.second);
   }
   ASSERT_TRUE(config.Rewrite().IsOK());
-  config.Load(path);
+  config.Load(CLIOptions(path));
   for (const auto &iter : mutable_cases) {
     auto s = config.Set(nullptr, iter.first, iter.second);
     ASSERT_TRUE(s.IsOK());
@@ -150,11 +150,11 @@ TEST(Config, Rewrite) {
 
   Config config;
   Redis::PopulateCommands();
-  ASSERT_TRUE(config.Load(path).IsOK());
+  ASSERT_TRUE(config.Load(CLIOptions(path)).IsOK());
   ASSERT_TRUE(config.Rewrite().IsOK());
   // Need to re-populate the command table since it has renamed by the previous
   Redis::PopulateCommands();
-  ASSERT_TRUE(config.Load(path).IsOK());
+  ASSERT_TRUE(config.Load(CLIOptions(path)).IsOK());
   unlink(path);
 }
 
@@ -163,7 +163,7 @@ TEST(Namespace, Add) {
   unlink(path);
 
   Config config;
-  config.Load(path);
+  config.Load(CLIOptions(path));
   config.slot_id_encoded = false;
   EXPECT_TRUE(!config.AddNamespace("ns", "t0").IsOK());
   config.requirepass = "foobared";
@@ -198,7 +198,7 @@ TEST(Namespace, Set) {
   unlink(path);
 
   Config config;
-  config.Load(path);
+  config.Load(CLIOptions(path));
   config.slot_id_encoded = false;
   config.requirepass = "foobared";
   std::vector<std::string> namespaces = {"n1", "n2", "n3", "n4"};
@@ -233,7 +233,7 @@ TEST(Namespace, Delete) {
   unlink(path);
 
   Config config;
-  config.Load(path);
+  config.Load(CLIOptions(path));
   config.slot_id_encoded = false;
   config.requirepass = "foobared";
   std::vector<std::string> namespaces = {"n1", "n2", "n3", "n4"};
@@ -259,7 +259,7 @@ TEST(Namespace, RewriteNamespaces) {
   const char *path = "test.conf";
   unlink(path);
   Config config;
-  config.Load(path);
+  config.Load(CLIOptions(path));
   config.requirepass = "test";
   config.backup_dir = "test";
   config.slot_id_encoded = false;
@@ -272,7 +272,7 @@ TEST(Namespace, RewriteNamespaces) {
   EXPECT_TRUE(config.DelNamespace("to-be-deleted-ns").IsOK());
 
   Config new_config;
-  auto s = new_config.Load(path);
+  auto s = new_config.Load(CLIOptions(path));
   for (size_t i = 0; i < namespaces.size(); i++) {
     std::string token;
     new_config.GetNamespace(namespaces[i], &token);
diff --git a/tests/gocase/integration/cli_options/cli_options_test.go 
b/tests/gocase/integration/cli_options/cli_options_test.go
new file mode 100644
index 00000000..d60a41de
--- /dev/null
+++ b/tests/gocase/integration/cli_options/cli_options_test.go
@@ -0,0 +1,39 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements.  See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership.  The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License.  You may obtain a copy of the License at
+*
+*   http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing,
+* software distributed under the License is distributed on an
+* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+* KIND, either express or implied.  See the License for the
+* specific language governing permissions and limitations
+* under the License.
+ */
+
+package clioptions
+
+import (
+       "context"
+       "testing"
+
+       "github.com/apache/incubator-kvrocks/tests/gocase/util"
+       "github.com/stretchr/testify/require"
+)
+
+func TestCLIOptions(t *testing.T) {
+       srv := util.StartServerWithCLIOptions(t, map[string]string{}, 
[]string{"--maxclients", "23333"})
+       defer srv.Close()
+
+       ctx := context.Background()
+       rdb := srv.NewClient()
+       defer func() { require.NoError(t, rdb.Close()) }()
+
+       require.Equal(t, "23333", rdb.ConfigGet(ctx, 
"maxclients").Val()["maxclients"])
+}
diff --git a/tests/gocase/util/server.go b/tests/gocase/util/server.go
index f6c80216..7bdfeec8 100644
--- a/tests/gocase/util/server.go
+++ b/tests/gocase/util/server.go
@@ -173,6 +173,10 @@ func StartTLSServer(t testing.TB, configs 
map[string]string) *KvrocksServer {
 }
 
 func StartServer(t testing.TB, configs map[string]string) *KvrocksServer {
+       return StartServerWithCLIOptions(t, configs, []string{})
+}
+
+func StartServerWithCLIOptions(t testing.TB, configs map[string]string, 
options []string) *KvrocksServer {
        b := *binPath
        require.NotEmpty(t, b, "please set the binary path by `-binPath`")
        cmd := exec.Command(b)
@@ -198,6 +202,7 @@ func StartServer(t testing.TB, configs map[string]string) 
*KvrocksServer {
        }
 
        cmd.Args = append(cmd.Args, "-c", f.Name())
+       cmd.Args = append(cmd.Args, options...)
 
        stdout, err := os.Create(filepath.Join(dir, "stdout"))
        require.NoError(t, err)

Reply via email to