Reply to empiredan's suggestion and modify my code.

apache · Jul 13, 2023 · d5cdf98 · d5cdf98
1 parent 4b838e2
commit d5cdf98
Show file tree

Hide file tree

Showing 6 changed files with 61 additions and 14 deletions.
diff --git a/src/base/pegasus_const.cpp b/src/base/pegasus_const.cpp
@@ -124,28 +124,30 @@ const std::unordered_map<std::string, cf_opts_setter> cf_opts_setters = {
     {ROCKSDB_WRITE_BUFFER_SIZE,
      [](const std::string &str, rocksdb::ColumnFamilyOptions &option) -> bool {
          uint64_t val = 0;
-         if (!dsn::buf2uint64(str, val))
+         if (!dsn::buf2uint64(str, val)) {
              return false;
+         }
          option.write_buffer_size = static_cast<size_t>(val);
          return true;
      }},
     {ROCKSDB_NUM_LEVELS,
      [](const std::string &str, rocksdb::ColumnFamilyOptions &option) -> bool {
          int32_t val = 0;
-         if (!dsn::buf2int32(str, val))
+         if (!dsn::buf2int32(str, val)) {
              return false;
+         }
          option.num_levels = val;
          return true;
      }},
 };
 
 const std::unordered_map<std::string, cf_opts_getter> cf_opts_getters = {
     {ROCKSDB_WRITE_BUFFER_SIZE,
-     [](/*out*/ std::string &str, const rocksdb::ColumnFamilyOptions &option) {
+     [](const rocksdb::ColumnFamilyOptions &option, /*out*/ std::string &str) {
          str = fmt::format("{}", option.write_buffer_size);
      }},
     {ROCKSDB_NUM_LEVELS,
-     [](/*out*/ std::string &str, const rocksdb::ColumnFamilyOptions &option) {
+     [](const rocksdb::ColumnFamilyOptions &option, /*out*/ std::string &str) {
          str = fmt::format("{}", option.num_levels);
      }},
 };

diff --git a/src/base/pegasus_const.h b/src/base/pegasus_const.h
@@ -92,6 +92,6 @@ using cf_opts_setter = std::function<bool(const std::string &, rocksdb::ColumnFa
 extern const std::unordered_map<std::string, cf_opts_setter> cf_opts_setters;
 
 using cf_opts_getter =
-    std::function<void(/*out*/ std::string &, const rocksdb::ColumnFamilyOptions &)>;
+    std::function<void(const rocksdb::ColumnFamilyOptions &, /*out*/ std::string &)>;
 extern const std::unordered_map<std::string, cf_opts_getter> cf_opts_getters;
 } // namespace pegasus
diff --git a/src/meta/test/meta_app_envs_test.cpp b/src/meta/test/meta_app_envs_test.cpp
@@ -209,7 +209,7 @@ TEST_F(meta_app_envs_test, update_app_envs_test)
         // Make sure all rocksdb options of ROCKSDB_DYNAMIC_OPTIONS are tested.
         // Hint: Mainly verify the update_rocksdb_dynamic_options function.
         std::map<std::string, std::string> all_test_envs;
-        for (auto test : tests) {
+        for (const auto &test : tests) {
             all_test_envs[test.env_key] = test.env_value;
         }
         for (const auto &option : replica_envs::ROCKSDB_DYNAMIC_OPTIONS) {

diff --git a/src/meta/test/meta_app_operation_test.cpp b/src/meta/test/meta_app_operation_test.cpp
@@ -367,9 +367,11 @@ TEST_F(meta_app_operation_test, create_app)
     // - create succeed with success_if_exist=true
     // - wrong rocksdb.num_levels (< 1)
     // - wrong rocksdb.num_levels (> 10)
+    // - wrong rocksdb.num_levels (non-digital character)
     // - create app with rocksdb.num_levels (= 5) succeed
     // - wrong rocksdb.write_buffer_size (< (16<<20))
     // - wrong rocksdb.write_buffer_size (> (512<<20))
+    // - wrong rocksdb.write_buffer_size (non-digital character)
     // - create app with rocksdb.write_buffer_size (= (32<<20)) succeed
     struct create_test
     {
@@ -441,6 +443,16 @@ TEST_F(meta_app_operation_test, create_app)
          app_status::AS_INVALID,
          ERR_INVALID_PARAMETERS,
          {{"rocksdb.num_levels", "11"}}},
+        {APP_NAME + "_11",
+         4,
+         3,
+         2,
+         3,
+         3,
+         false,
+         app_status::AS_INVALID,
+         ERR_INVALID_PARAMETERS,
+         {{"rocksdb.num_levels", "5i"}}},
         {APP_NAME + "_11",
          4,
          3,
@@ -471,6 +483,16 @@ TEST_F(meta_app_operation_test, create_app)
          app_status::AS_INVALID,
          ERR_INVALID_PARAMETERS,
          {{"rocksdb.write_buffer_size", "1073741824"}}},
+        {APP_NAME,
+         4,
+         3,
+         2,
+         3,
+         3,
+         false,
+         app_status::AS_INVALID,
+         ERR_INVALID_PARAMETERS,
+         {{"rocksdb.write_buffer_size", "n33554432"}}},
         {APP_NAME + "_12",
          4,
          3,
@@ -539,7 +561,7 @@ TEST_F(meta_app_operation_test, create_app)
         // Make sure all rocksdb options of ROCKSDB_DYNAMIC_OPTIONS and ROCKSDB_STATIC_OPTIONS are
         // tested. Hint: Mainly verify the validate_app_envs function.
         std::map<std::string, std::string> all_test_envs;
-        for (auto test : tests) {
+        for (const auto &test : tests) {
             all_test_envs.insert(test.envs.begin(), test.envs.end());
         }
         for (const auto &option : replica_envs::ROCKSDB_DYNAMIC_OPTIONS) {

diff --git a/src/server/pegasus_server_impl.cpp b/src/server/pegasus_server_impl.cpp
@@ -2644,6 +2644,7 @@ void pegasus_server_impl::update_rocksdb_dynamic_options(
         std::vector<std::string> args;
         // split_args example: Parse "write_buffer_size" from "rocksdb.write_buffer_size"
         dsn::utils::split_args(option.c_str(), args, '.');
+        CHECK_EQ(args.size(), 2);
         new_options[args[1]] = find->second;
     }
 
@@ -2731,8 +2732,31 @@ void pegasus_server_impl::query_app_envs(/*out*/ std::map<std::string, std::stri
 
     // Get Data ColumnFamilyOptions directly from _data_cf
     rocksdb::ColumnFamilyDescriptor desc;
-    rocksdb::Status s = _data_cf->GetDescriptor(&desc);
-    envs[ROCKSDB_NUM_LEVELS] = fmt::format("{}", desc.options.num_levels);
+    auto s = _data_cf->GetDescriptor(&desc);
+    CHECK_TRUE(s.ok());
+    for (const auto &option : pegasus::ROCKSDB_STATIC_OPTIONS) {
+        auto getter = cf_opts_getters.find(option);
+        if (getter == cf_opts_getters.end()) {
+            LOG_WARNING("cannot find {} getter function, and get this option fail.", option);
+            continue;
+        }
+        std::string option_val;
+        getter->second(desc.options, option_val);
+        envs[option] = option_val;
+    }
+    for (const auto &option : pegasus::ROCKSDB_DYNAMIC_OPTIONS) {
+        if (option.compare(ROCKSDB_WRITE_BUFFER_SIZE) == 0) {
+            continue;
+        }
+        auto getter = cf_opts_getters.find(option);
+        if (getter == cf_opts_getters.end()) {
+            LOG_WARNING("cannot find {} getter function, and get this option fail.", option);
+            continue;
+        }
+        std::string option_val;
+        getter->second(desc.options, option_val);
+        envs[option] = option_val;
+    }
 }
 
 void pegasus_server_impl::update_usage_scenario(const std::map<std::string, std::string> &envs)

diff --git a/src/server/test/pegasus_server_impl_test.cpp b/src/server/test/pegasus_server_impl_test.cpp
@@ -113,7 +113,7 @@ class pegasus_server_impl_test : public pegasus_server_test_base
         {
             // Make sure all rocksdb options of ROCKSDB_DYNAMIC_OPTIONS and ROCKSDB_STATIC_OPTIONS
             // are tested.
-            for (auto test : tests) {
+            for (const auto &test : tests) {
                 all_test_envs[test.env_key] = test.env_value;
             }
             for (const auto &option : pegasus::ROCKSDB_DYNAMIC_OPTIONS) {
@@ -132,13 +132,12 @@ class pegasus_server_impl_test : public pegasus_server_test_base
 
         std::map<std::string, std::string> query_envs;
         _server->query_app_envs(query_envs);
-        for (auto test : tests) {
-            auto iter = query_envs.find(test.env_key);
+        for (const auto &test : tests) {
+            const auto &iter = query_envs.find(test.env_key);
             if (iter != query_envs.end()) {
                 ASSERT_EQ(iter->second, test.expect_value);
             } else {
-                ASSERT_EQ(test.env_key,
-                          fmt::format("query_app_envs not supported {}", test.env_key));
+                ASSERT_TRUE(false) << fmt::format("query_app_envs not supported {}", test.env_key);
             }
         }
     }