From d0196c184166fc906dc4c4051e979590f2b9f354 Mon Sep 17 00:00:00 2001 From: Siyang Tang Date: Mon, 30 Dec 2024 07:48:46 +0800 Subject: [PATCH] fix review --- cloud/src/common/configbase.cpp | 53 +++++++---- cloud/src/common/configbase.h | 12 ++- cloud/src/meta-service/meta_service_http.cpp | 17 +++- cloud/test/meta_service_http_test.cpp | 97 +++++++++++++++----- 4 files changed, 132 insertions(+), 47 deletions(-) diff --git a/cloud/src/common/configbase.cpp b/cloud/src/common/configbase.cpp index 01494426334863a..1f726f2b6cdbe85 100644 --- a/cloud/src/common/configbase.cpp +++ b/cloud/src/common/configbase.cpp @@ -15,6 +15,8 @@ // specific language governing permissions and limitations // under the License. +#include + #include #include #include @@ -25,6 +27,7 @@ #include #include #include +#include #define __IN_CONFIGBASE_CPP__ #include "common/config.h" @@ -410,35 +413,49 @@ bool do_set_config(const Register::Field& feild, const std::string& value, bool return false; } -bool set_config(const std::string& field, const std::string& value, bool need_persist, - const std::string& custom_conf_path) { +std::pair set_config(const std::string& field, const std::string& value, + bool need_persist, Properties& props) { auto it = Register::_s_field_map->find(field); if (it == Register::_s_field_map->end()) { - return false; + return {false, fmt::format("config field={} not exists", field)}; } if (!it->second.valmutable) { - return false; + return {false, fmt::format("config field={} is immutable", field)}; } + if (!do_set_config(it->second, value, need_persist, props)) { + return {false, fmt::format("not supported to modify field={}, value={}", field, value)}; + } + return {true, {}}; +} + +std::pair set_config(std::unordered_map field_map, + bool need_persist, const std::string& custom_conf_path) { Properties props; - auto set_conf_func = [&]() { - if (!do_set_config(it->second, value, need_persist, props)) { - std::cerr << "not supported to modify: " << field << "=" << value << std::endl; - return false; + auto set_conf_closure = [&]() -> std::pair { + for (const auto& [field, value] : field_map) { + if (auto [succ, cause] = set_config(field, value, need_persist, props); !succ) { + return {false, std::move(cause)}; + } } - return true; + return {true, {}}; }; - if (need_persist) { - // lock to make sure only one thread can modify the be_custom.conf - std::lock_guard l(conf_persist_lock); - if (!set_conf_func()) { - return false; - } - return props.dump(custom_conf_path); + if (!need_persist) { + return set_conf_closure(); } - return set_conf_func(); + // lock to make sure only one thread can modify the conf file + std::lock_guard l(conf_persist_lock); + auto [succ, cause] = set_conf_closure(); + if (!succ) { + return {succ, std::move(cause)}; + } + if (props.dump(custom_conf_path)) { + return {true, {}}; + } + return {false, fmt::format("dump config modification to custom_conf_path={} " + "failed, plz check config::custom_conf_path and io status", + custom_conf_path)}; } - } // namespace doris::cloud::config diff --git a/cloud/src/common/configbase.h b/cloud/src/common/configbase.h index 1216d8979a65eb4..8419021fb5981aa 100644 --- a/cloud/src/common/configbase.h +++ b/cloud/src/common/configbase.h @@ -22,6 +22,7 @@ #include #include #include +#include #include namespace doris::cloud::config { @@ -152,7 +153,11 @@ class Properties { void set_force(const std::string& key, const std::string& val); - // dump props to conf file + // Dump props to conf file if conffile exists, will append to it + // else will dump to a new conffile. + // + // This Function will generate a tmp file for modification and rename it + // to subtitute the original one if modication suncess. bool dump(const std::string& conffile); private: @@ -170,6 +175,7 @@ extern std::map* full_conf_map; bool init(const char* conf_file, bool fill_conf_map = false, bool must_exist = true, bool set_to_default = true); -bool set_config(const std::string& field, const std::string& value, bool need_persist, - const std::string& custom_conf_path); +std::pair set_config(std::unordered_map field_map, + bool need_persist, const std::string& custom_conf_path); + } // namespace doris::cloud::config diff --git a/cloud/src/meta-service/meta_service_http.cpp b/cloud/src/meta-service/meta_service_http.cpp index 2b885ccdafcc134..78f2dc03a959f15 100644 --- a/cloud/src/meta-service/meta_service_http.cpp +++ b/cloud/src/meta-service/meta_service_http.cpp @@ -455,23 +455,32 @@ static HttpResponse process_update_config(MetaServiceImpl* service, brpc::Contro const auto& uri = cntl->http_request().uri(); bool persist = (http_query(uri, "persist") == "true"); auto configs = std::string {http_query(uri, "configs")}; + auto reason = std::string {http_query(uri, "reason")}; + LOG(INFO) << "modify configs for reason=" << reason << ", configs=" << configs + << ", persist=" << http_query(uri, "persist"); if (configs.empty()) [[unlikely]] { + LOG(WARNING) << "query param `config` should not be empty"; return http_json_reply(MetaServiceCode::INVALID_ARGUMENT, "query param `config` should not be empty"); } + std::unordered_map conf_map; auto conf_list = split(configs, ','); for (const auto& conf : conf_list) { auto conf_pair = split(conf, '='); if (conf_pair.size() != 2) [[unlikely]] { + LOG(WARNING) << "failed to split config=[{}] from `k=v` pattern" << conf; return http_json_reply(MetaServiceCode::INVALID_ARGUMENT, fmt::format("config {} is invalid", configs)); } trim(conf_pair[0]); trim(conf_pair[1]); - if (!config::set_config(conf_pair[0], conf_pair[1], persist, config::custom_conf_path)) { - return http_json_reply(MetaServiceCode::INVALID_ARGUMENT, - fmt::format("set {}={} failed", conf_pair[0], conf_pair[1])); - } + conf_map.emplace(std::move(conf_pair[0]), std::move(conf_pair[1])); + } + if (auto [succ, cause] = + config::set_config(std::move(conf_map), persist, config::custom_conf_path); + !succ) { + LOG(WARNING) << cause; + return http_json_reply(MetaServiceCode::INVALID_ARGUMENT, cause); } return http_json_reply(MetaServiceCode::OK, ""); } diff --git a/cloud/test/meta_service_http_test.cpp b/cloud/test/meta_service_http_test.cpp index 9454ededfd680dc..af6d50fa9cb8569 100644 --- a/cloud/test/meta_service_http_test.cpp +++ b/cloud/test/meta_service_http_test.cpp @@ -1646,7 +1646,14 @@ TEST(MetaServiceHttpTest, UpdateConfig) { { auto [status_code, content] = ctx.query("update_config", "configs=aaa=bbb"); ASSERT_EQ(status_code, 400); - std::string msg = "set aaa=bbb failed"; + std::string msg = "config field=aaa not exists"; + ASSERT_NE(content.find(msg), std::string::npos); + } + { + auto [status_code, content] = + ctx.query("update_config", "configs=custom_conf_path=./doris_conf"); + ASSERT_EQ(status_code, 400); + std::string msg = "config field=custom_conf_path is immutable"; ASSERT_NE(content.find(msg), std::string::npos); } { @@ -1663,31 +1670,77 @@ TEST(MetaServiceHttpTest, UpdateConfig) { ASSERT_EQ(config::recycle_interval_seconds, 3601); } { - auto original_conf_path = config::custom_conf_path; - config::custom_conf_path = "./doris_cloud.conf"; - auto [status_code, content] = ctx.query( - "update_config", - "configs=recycle_interval_seconds=3659,retention_seconds=259219&persist=true"); + auto [status_code, content] = + ctx.query("update_config", "configs=enable_s3_rate_limiter=true"); ASSERT_EQ(status_code, 200); - ASSERT_EQ(config::recycle_interval_seconds, 3659); - ASSERT_EQ(config::retention_seconds, 259219); - config::Properties props; - ASSERT_TRUE(props.load(config::custom_conf_path.c_str(), true)); + ASSERT_TRUE(config::enable_s3_rate_limiter); + } + { + auto [status_code, content] = + ctx.query("update_config", "enable_s3_rate_limiter=invalid"); + ASSERT_EQ(status_code, 400); + } + { + auto original_conf_path = config::custom_conf_path; + config::custom_conf_path = "./doris_cloud_custom.conf"; { - bool new_val_set = false; - int64_t recycle_interval_s = 0; - ASSERT_TRUE(props.get_or_default("recycle_interval_seconds", nullptr, - recycle_interval_s, &new_val_set)); - ASSERT_TRUE(new_val_set); - ASSERT_EQ(recycle_interval_s, 3659); + auto [status_code, content] = ctx.query( + "update_config", + "configs=recycle_interval_seconds=3659,retention_seconds=259219&persist=true"); + ASSERT_EQ(status_code, 200); + ASSERT_EQ(config::recycle_interval_seconds, 3659); + ASSERT_EQ(config::retention_seconds, 259219); + config::Properties props; + ASSERT_TRUE(props.load(config::custom_conf_path.c_str(), true)); + { + bool new_val_set = false; + int64_t recycle_interval_s = 0; + ASSERT_TRUE(props.get_or_default("recycle_interval_seconds", nullptr, + recycle_interval_s, &new_val_set)); + ASSERT_TRUE(new_val_set); + ASSERT_EQ(recycle_interval_s, 3659); + } + { + bool new_val_set = false; + int64_t retention_s = 0; + ASSERT_TRUE(props.get_or_default("retention_seconds", nullptr, retention_s, + &new_val_set)); + ASSERT_TRUE(new_val_set); + ASSERT_EQ(retention_s, 259219); + } } { - bool new_val_set = false; - int64_t retention_s = 0; - ASSERT_TRUE( - props.get_or_default("retention_seconds", nullptr, retention_s, &new_val_set)); - ASSERT_TRUE(new_val_set); - ASSERT_EQ(retention_s, 259219); + auto [status_code, content] = ctx.query( + "update_config", "configs=enable_s3_rate_limiter=false&persist=true"); + ASSERT_EQ(status_code, 200); + ASSERT_EQ(config::recycle_interval_seconds, 3659); + ASSERT_EQ(config::retention_seconds, 259219); + config::Properties props; + ASSERT_TRUE(props.load(config::custom_conf_path.c_str(), true)); + { + bool new_val_set = false; + int64_t recycle_interval_s = 0; + ASSERT_TRUE(props.get_or_default("recycle_interval_seconds", nullptr, + recycle_interval_s, &new_val_set)); + ASSERT_TRUE(new_val_set); + ASSERT_EQ(recycle_interval_s, 3659); + } + { + bool new_val_set = false; + int64_t retention_s = 0; + ASSERT_TRUE(props.get_or_default("retention_seconds", nullptr, retention_s, + &new_val_set)); + ASSERT_TRUE(new_val_set); + ASSERT_EQ(retention_s, 259219); + } + { + bool new_val_set = false; + bool enable_s3_rate_limiter = true; + ASSERT_TRUE(props.get_or_default("enable_s3_rate_limiter", nullptr, + enable_s3_rate_limiter, &new_val_set)); + ASSERT_TRUE(new_val_set); + ASSERT_FALSE(enable_s3_rate_limiter); + } } std::filesystem::remove(config::custom_conf_path); config::custom_conf_path = original_conf_path;