Skip to content

Commit 2de1f97

Browse files
TangSiyang2001TangSiyang2001
committed
fix review
1 parent 64ca066 commit 2de1f97

File tree

4 files changed

+132
-48
lines changed

4 files changed

+132
-48
lines changed

‎cloud/src/common/configbase.cpp

+35-19
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
// specific language governing permissions and limitations
1616
// under the License.
1717

18+
#include <fmt/core.h>
19+
1820
#include <algorithm>
1921
#include <cerrno>
2022
#include <cstring>
@@ -25,6 +27,7 @@
2527
#include <map>
2628
#include <mutex>
2729
#include <sstream>
30+
#include <utility>
2831

2932
#define __IN_CONFIGBASE_CPP__
3033
#include "common/config.h"
@@ -228,7 +231,6 @@ bool Properties::load(const char* conf_file, bool must_exist) {
228231
return true;
229232
}
230233

231-
// dump props to conf file
232234
bool Properties::dump(const std::string& conffile) {
233235
std::string conffile_tmp = conffile + ".tmp";
234236

@@ -410,35 +412,49 @@ bool do_set_config(const Register::Field& feild, const std::string& value, bool
410412
return false;
411413
}
412414

413-
bool set_config(const std::string& field, const std::string& value, bool need_persist,
414-
const std::string& custom_conf_path) {
415+
std::pair<bool, std::string> set_config(const std::string& field, const std::string& value,
416+
bool need_persist, Properties& props) {
415417
auto it = Register::_s_field_map->find(field);
416418
if (it == Register::_s_field_map->end()) {
417-
return false;
419+
return {false, fmt::format("config field={} not exists", field)};
418420
}
419421
if (!it->second.valmutable) {
420-
return false;
422+
return {false, fmt::format("config field={} is immutable", field)};
421423
}
422424

425+
if (!do_set_config(it->second, value, need_persist, props)) {
426+
return {false, fmt::format("not supported to modify field={}, value={}", field, value)};
427+
}
428+
return {true, {}};
429+
}
430+
431+
std::pair<bool, std::string> set_config(std::unordered_map<std::string, std::string> field_map,
432+
bool need_persist, const std::string& custom_conf_path) {
423433
Properties props;
424-
auto set_conf_func = [&]() {
425-
if (!do_set_config(it->second, value, need_persist, props)) {
426-
std::cerr << "not supported to modify: " << field << "=" << value << std::endl;
427-
return false;
434+
auto set_conf_closure = [&]() -> std::pair<bool, std::string> {
435+
for (const auto& [field, value] : field_map) {
436+
if (auto [succ, cause] = set_config(field, value, need_persist, props); !succ) {
437+
return {false, std::move(cause)};
438+
}
428439
}
429-
return true;
440+
return {true, {}};
430441
};
431442

432-
if (need_persist) {
433-
// lock to make sure only one thread can modify the be_custom.conf
434-
std::lock_guard<std::mutex> l(conf_persist_lock);
435-
if (!set_conf_func()) {
436-
return false;
437-
}
438-
return props.dump(custom_conf_path);
443+
if (!need_persist) {
444+
return set_conf_closure();
439445
}
440446

441-
return set_conf_func();
447+
// lock to make sure only one thread can modify the conf file
448+
std::lock_guard<std::mutex> l(conf_persist_lock);
449+
auto [succ, cause] = set_conf_closure();
450+
if (!succ) {
451+
return {succ, std::move(cause)};
452+
}
453+
if (props.dump(custom_conf_path)) {
454+
return {true, {}};
455+
}
456+
return {false, fmt::format("dump config modification to custom_conf_path={} "
457+
"failed, plz check config::custom_conf_path and io status",
458+
custom_conf_path)};
442459
}
443-
444460
} // namespace doris::cloud::config

‎cloud/src/common/configbase.h

+9-3
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
#include <map>
2323
#include <mutex>
2424
#include <string>
25+
#include <utility>
2526
#include <vector>
2627

2728
namespace doris::cloud::config {
@@ -152,7 +153,11 @@ class Properties {
152153

153154
void set_force(const std::string& key, const std::string& val);
154155

155-
// dump props to conf file
156+
// Dump props to conf file if conffile exists, will append to it
157+
// else will dump to a new conffile.
158+
//
159+
// This Function will generate a tmp file for modification and rename it
160+
// to subtitute the original one if modication success.
156161
bool dump(const std::string& conffile);
157162

158163
private:
@@ -170,6 +175,7 @@ extern std::map<std::string, std::string>* full_conf_map;
170175
bool init(const char* conf_file, bool fill_conf_map = false, bool must_exist = true,
171176
bool set_to_default = true);
172177

173-
bool set_config(const std::string& field, const std::string& value, bool need_persist,
174-
const std::string& custom_conf_path);
178+
std::pair<bool, std::string> set_config(std::unordered_map<std::string, std::string> field_map,
179+
bool need_persist, const std::string& custom_conf_path);
180+
175181
} // namespace doris::cloud::config

‎cloud/src/meta-service/meta_service_http.cpp

+13-4
Original file line numberDiff line numberDiff line change
@@ -455,23 +455,32 @@ static HttpResponse process_update_config(MetaServiceImpl* service, brpc::Contro
455455
const auto& uri = cntl->http_request().uri();
456456
bool persist = (http_query(uri, "persist") == "true");
457457
auto configs = std::string {http_query(uri, "configs")};
458+
auto reason = std::string {http_query(uri, "reason")};
459+
LOG(INFO) << "modify configs for reason=" << reason << ", configs=" << configs
460+
<< ", persist=" << http_query(uri, "persist");
458461
if (configs.empty()) [[unlikely]] {
462+
LOG(WARNING) << "query param `config` should not be empty";
459463
return http_json_reply(MetaServiceCode::INVALID_ARGUMENT,
460464
"query param `config` should not be empty");
461465
}
466+
std::unordered_map<std::string, std::string> conf_map;
462467
auto conf_list = split(configs, ',');
463468
for (const auto& conf : conf_list) {
464469
auto conf_pair = split(conf, '=');
465470
if (conf_pair.size() != 2) [[unlikely]] {
471+
LOG(WARNING) << "failed to split config=[{}] from `k=v` pattern" << conf;
466472
return http_json_reply(MetaServiceCode::INVALID_ARGUMENT,
467473
fmt::format("config {} is invalid", configs));
468474
}
469475
trim(conf_pair[0]);
470476
trim(conf_pair[1]);
471-
if (!config::set_config(conf_pair[0], conf_pair[1], persist, config::custom_conf_path)) {
472-
return http_json_reply(MetaServiceCode::INVALID_ARGUMENT,
473-
fmt::format("set {}={} failed", conf_pair[0], conf_pair[1]));
474-
}
477+
conf_map.emplace(std::move(conf_pair[0]), std::move(conf_pair[1]));
478+
}
479+
if (auto [succ, cause] =
480+
config::set_config(std::move(conf_map), persist, config::custom_conf_path);
481+
!succ) {
482+
LOG(WARNING) << cause;
483+
return http_json_reply(MetaServiceCode::INVALID_ARGUMENT, cause);
475484
}
476485
return http_json_reply(MetaServiceCode::OK, "");
477486
}

‎cloud/test/meta_service_http_test.cpp

+75-22
Original file line numberDiff line numberDiff line change
@@ -1646,7 +1646,14 @@ TEST(MetaServiceHttpTest, UpdateConfig) {
16461646
{
16471647
auto [status_code, content] = ctx.query<std::string>("update_config", "configs=aaa=bbb");
16481648
ASSERT_EQ(status_code, 400);
1649-
std::string msg = "set aaa=bbb failed";
1649+
std::string msg = "config field=aaa not exists";
1650+
ASSERT_NE(content.find(msg), std::string::npos);
1651+
}
1652+
{
1653+
auto [status_code, content] =
1654+
ctx.query<std::string>("update_config", "configs=custom_conf_path=./doris_conf");
1655+
ASSERT_EQ(status_code, 400);
1656+
std::string msg = "config field=custom_conf_path is immutable";
16501657
ASSERT_NE(content.find(msg), std::string::npos);
16511658
}
16521659
{
@@ -1663,31 +1670,77 @@ TEST(MetaServiceHttpTest, UpdateConfig) {
16631670
ASSERT_EQ(config::recycle_interval_seconds, 3601);
16641671
}
16651672
{
1666-
auto original_conf_path = config::custom_conf_path;
1667-
config::custom_conf_path = "./doris_cloud.conf";
1668-
auto [status_code, content] = ctx.query<std::string>(
1669-
"update_config",
1670-
"configs=recycle_interval_seconds=3659,retention_seconds=259219&persist=true");
1673+
auto [status_code, content] =
1674+
ctx.query<std::string>("update_config", "configs=enable_s3_rate_limiter=true");
16711675
ASSERT_EQ(status_code, 200);
1672-
ASSERT_EQ(config::recycle_interval_seconds, 3659);
1673-
ASSERT_EQ(config::retention_seconds, 259219);
1674-
config::Properties props;
1675-
ASSERT_TRUE(props.load(config::custom_conf_path.c_str(), true));
1676+
ASSERT_TRUE(config::enable_s3_rate_limiter);
1677+
}
1678+
{
1679+
auto [status_code, content] =
1680+
ctx.query<std::string>("update_config", "enable_s3_rate_limiter=invalid");
1681+
ASSERT_EQ(status_code, 400);
1682+
}
1683+
{
1684+
auto original_conf_path = config::custom_conf_path;
1685+
config::custom_conf_path = "./doris_cloud_custom.conf";
16761686
{
1677-
bool new_val_set = false;
1678-
int64_t recycle_interval_s = 0;
1679-
ASSERT_TRUE(props.get_or_default("recycle_interval_seconds", nullptr,
1680-
recycle_interval_s, &new_val_set));
1681-
ASSERT_TRUE(new_val_set);
1682-
ASSERT_EQ(recycle_interval_s, 3659);
1687+
auto [status_code, content] = ctx.query<std::string>(
1688+
"update_config",
1689+
"configs=recycle_interval_seconds=3659,retention_seconds=259219&persist=true");
1690+
ASSERT_EQ(status_code, 200);
1691+
ASSERT_EQ(config::recycle_interval_seconds, 3659);
1692+
ASSERT_EQ(config::retention_seconds, 259219);
1693+
config::Properties props;
1694+
ASSERT_TRUE(props.load(config::custom_conf_path.c_str(), true));
1695+
{
1696+
bool new_val_set = false;
1697+
int64_t recycle_interval_s = 0;
1698+
ASSERT_TRUE(props.get_or_default("recycle_interval_seconds", nullptr,
1699+
recycle_interval_s, &new_val_set));
1700+
ASSERT_TRUE(new_val_set);
1701+
ASSERT_EQ(recycle_interval_s, 3659);
1702+
}
1703+
{
1704+
bool new_val_set = false;
1705+
int64_t retention_s = 0;
1706+
ASSERT_TRUE(props.get_or_default("retention_seconds", nullptr, retention_s,
1707+
&new_val_set));
1708+
ASSERT_TRUE(new_val_set);
1709+
ASSERT_EQ(retention_s, 259219);
1710+
}
16831711
}
16841712
{
1685-
bool new_val_set = false;
1686-
int64_t retention_s = 0;
1687-
ASSERT_TRUE(
1688-
props.get_or_default("retention_seconds", nullptr, retention_s, &new_val_set));
1689-
ASSERT_TRUE(new_val_set);
1690-
ASSERT_EQ(retention_s, 259219);
1713+
auto [status_code, content] = ctx.query<std::string>(
1714+
"update_config", "configs=enable_s3_rate_limiter=false&persist=true");
1715+
ASSERT_EQ(status_code, 200);
1716+
ASSERT_EQ(config::recycle_interval_seconds, 3659);
1717+
ASSERT_EQ(config::retention_seconds, 259219);
1718+
config::Properties props;
1719+
ASSERT_TRUE(props.load(config::custom_conf_path.c_str(), true));
1720+
{
1721+
bool new_val_set = false;
1722+
int64_t recycle_interval_s = 0;
1723+
ASSERT_TRUE(props.get_or_default("recycle_interval_seconds", nullptr,
1724+
recycle_interval_s, &new_val_set));
1725+
ASSERT_TRUE(new_val_set);
1726+
ASSERT_EQ(recycle_interval_s, 3659);
1727+
}
1728+
{
1729+
bool new_val_set = false;
1730+
int64_t retention_s = 0;
1731+
ASSERT_TRUE(props.get_or_default("retention_seconds", nullptr, retention_s,
1732+
&new_val_set));
1733+
ASSERT_TRUE(new_val_set);
1734+
ASSERT_EQ(retention_s, 259219);
1735+
}
1736+
{
1737+
bool new_val_set = false;
1738+
bool enable_s3_rate_limiter = true;
1739+
ASSERT_TRUE(props.get_or_default("enable_s3_rate_limiter", nullptr,
1740+
enable_s3_rate_limiter, &new_val_set));
1741+
ASSERT_TRUE(new_val_set);
1742+
ASSERT_FALSE(enable_s3_rate_limiter);
1743+
}
16911744
}
16921745
std::filesystem::remove(config::custom_conf_path);
16931746
config::custom_conf_path = original_conf_path;

0 commit comments

Comments
 (0)