Skip to content

Commit

Permalink
fix review
Browse files Browse the repository at this point in the history
  • Loading branch information
@TangSiyang2001
TangSiyang2001 committed Jan 4, 2025
1 parent 64ca066 commit d0196c1
Show file tree
Hide file tree
Showing 4 changed files with 132 additions and 47 deletions.
53 changes: 35 additions & 18 deletions cloud/src/common/configbase.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
// specific language governing permissions and limitations
// under the License.

#include <fmt/core.h>

#include <algorithm>
#include <cerrno>
#include <cstring>
Expand All @@ -25,6 +27,7 @@
#include <map>
#include <mutex>
#include <sstream>
#include <utility>

#define __IN_CONFIGBASE_CPP__
#include "common/config.h"
Expand Down Expand Up @@ -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<bool, std::string> 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<bool, std::string> set_config(std::unordered_map<std::string, std::string> 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<bool, std::string> {
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<std::mutex> 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<std::mutex> 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
12 changes: 9 additions & 3 deletions cloud/src/common/configbase.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <map>
#include <mutex>
#include <string>
#include <utility>
#include <vector>

namespace doris::cloud::config {
Expand Down Expand Up @@ -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:
Expand All @@ -170,6 +175,7 @@ extern std::map<std::string, std::string>* 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<bool, std::string> set_config(std::unordered_map<std::string, std::string> field_map,
bool need_persist, const std::string& custom_conf_path);

} // namespace doris::cloud::config
17 changes: 13 additions & 4 deletions cloud/src/meta-service/meta_service_http.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string, std::string> 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, "");
}
Expand Down
97 changes: 75 additions & 22 deletions cloud/test/meta_service_http_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1646,7 +1646,14 @@ TEST(MetaServiceHttpTest, UpdateConfig) {
{
auto [status_code, content] = ctx.query<std::string>("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<std::string>("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);
}
{
Expand All @@ -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<std::string>(
"update_config",
"configs=recycle_interval_seconds=3659,retention_seconds=259219&persist=true");
auto [status_code, content] =
ctx.query<std::string>("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<std::string>("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<std::string>(
"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<std::string>(
"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;
Expand Down

0 comments on commit d0196c1

Please sign in to comment.