acelyc111 commented on code in PR #1272:
URL:
https://github.com/apache/incubator-pegasus/pull/1272#discussion_r1039769684
##########
src/meta/test/meta_app_operation_test.cpp:
##########
@@ -795,5 +809,28 @@ TEST_F(meta_app_operation_test,
recover_from_max_replica_count_env)
verify_app_max_replica_count(APP_NAME, new_max_replica_count);
}
+TEST_F(meta_app_operation_test, rename_app)
+{
+ const std::string app_name_1 = APP_NAME + "_rename_1";
+ create_app(app_name_1);
+ auto app = find_app(app_name_1);
Review Comment:
Better to check `app` can be found by `ASSERT_TRUE(app);`
##########
src/meta/test/meta_app_operation_test.cpp:
##########
@@ -795,5 +809,28 @@ TEST_F(meta_app_operation_test,
recover_from_max_replica_count_env)
verify_app_max_replica_count(APP_NAME, new_max_replica_count);
}
+TEST_F(meta_app_operation_test, rename_app)
+{
+ const std::string app_name_1 = APP_NAME + "_rename_1";
+ create_app(app_name_1);
+ auto app = find_app(app_name_1);
+ auto app_id_1 = app->app_id;
+
+ const std::string app_name_2 = APP_NAME + "_rename_2";
+ create_app(app_name_2);
+ app = find_app(app_name_2);
Review Comment:
Same.
##########
src/meta/test/meta_app_operation_test.cpp:
##########
@@ -795,5 +809,28 @@ TEST_F(meta_app_operation_test,
recover_from_max_replica_count_env)
verify_app_max_replica_count(APP_NAME, new_max_replica_count);
}
+TEST_F(meta_app_operation_test, rename_app)
+{
+ const std::string app_name_1 = APP_NAME + "_rename_1";
+ create_app(app_name_1);
+ auto app = find_app(app_name_1);
+ auto app_id_1 = app->app_id;
+
+ const std::string app_name_2 = APP_NAME + "_rename_2";
+ create_app(app_name_2);
+ app = find_app(app_name_2);
+ auto app_id_2 = app->app_id;
+
+ const std::string app_name_3 = APP_NAME + "_rename_3";
+
+ auto resp = rename_app(app_name_1, app_name_2);
+ ASSERT_EQ(resp.err, ERR_INVALID_PARAMETERS);
+
+ resp = rename_app(app_name_1, app_name_3);
+ ASSERT_EQ(resp.err, ERR_OK);
+ app = find_app(app_name_3);
+ ASSERT_EQ(app->app_id, app_id_1);
Review Comment:
For ASSERT_EQ, it's recommand to make the first arg as the "expect value",
and the second as the "actual value", that's to say update to
`ASSERT_EQ(app_id_1, app->app_id);`
##########
src/meta/test/meta_app_operation_test.cpp:
##########
@@ -795,5 +809,28 @@ TEST_F(meta_app_operation_test,
recover_from_max_replica_count_env)
verify_app_max_replica_count(APP_NAME, new_max_replica_count);
}
+TEST_F(meta_app_operation_test, rename_app)
+{
+ const std::string app_name_1 = APP_NAME + "_rename_1";
+ create_app(app_name_1);
+ auto app = find_app(app_name_1);
+ auto app_id_1 = app->app_id;
+
+ const std::string app_name_2 = APP_NAME + "_rename_2";
+ create_app(app_name_2);
+ app = find_app(app_name_2);
+ auto app_id_2 = app->app_id;
+
+ const std::string app_name_3 = APP_NAME + "_rename_3";
+
+ auto resp = rename_app(app_name_1, app_name_2);
+ ASSERT_EQ(resp.err, ERR_INVALID_PARAMETERS);
+
+ resp = rename_app(app_name_1, app_name_3);
+ ASSERT_EQ(resp.err, ERR_OK);
+ app = find_app(app_name_3);
Review Comment:
Same, check app can be found.
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,111 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ const auto &old_app_name = rpc.request().old_app_name;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ zauto_read_lock l;
+ auto target_app = get_app(old_app_name);
+
+ if (target_app == nullptr) {
+ auto &response = rpc.response();
+ std::ostringstream oss;
+ response.err = ERR_APP_NOT_EXIST;
+ oss << "ERROR: app(" << old_app_name << ") not exist. check it!" <<
std::endl;
+ response.hint_message += oss.str();
+ return;
+ }
+
+ LOG_INFO_F("ready to update remote app_name: app_id={}, old_app_name={},
new_app_name={}.",
+ target_app->app_id,
+ old_app_name,
+ new_app_name);
+
+ auto ainfo = *(reinterpret_cast<app_info *>(target_app.get()));
+ ainfo.app_name = new_app_name;
+ auto app_path = get_app_path(*target_app);
+
+ do_update_app_info(app_path, ainfo, [this, target_app, rpc](error_code ec)
mutable {
+ const auto &new_app_name = rpc.request().new_app_name;
+ zauto_write_lock l(_lock);
+
+ CHECK_EQ(ec, ERR_OK);
+
+ const auto old_app_name = target_app->app_name;
+ target_app->app_name = new_app_name;
+
+ _exist_apps.emplace(new_app_name, target_app);
+ _exist_apps.erase(old_app_name);
+
+ LOG_INFO_F("both remote and local env of app_name have been updated "
+ "successfully: app_id={}, old_app_name={}, new_app_name={}",
+ target_app->app_id,
+ old_app_name,
+ new_app_name);
+
+ auto &response = rpc.response();
+ response.err = ERR_OK;
+
+ });
+}
+
+void server_state::rename_app(configuration_rename_app_rpc rpc)
+{
+ const std::string &old_app_name = rpc.request().old_app_name;
+ const std::string &new_app_name = rpc.request().new_app_name;
+ auto &response = rpc.response();
+
+ std::shared_ptr<app_state> target_app;
+ bool do_rename = false;
+ LOG_INFO_F(
+ "rename app request, old_app_name({}), new_app_name({})",
old_app_name, new_app_name);
+
+ zauto_read_lock l(_lock);
+ target_app = get_app(old_app_name);
+
+ std::ostringstream oss;
+ if (target_app == nullptr) {
+ response.err = ERR_APP_NOT_EXIST;
+ oss << "ERROR: app(" << old_app_name << ") not exist. check it!" <<
std::endl;
+ response.hint_message += oss.str();
Review Comment:
This is the only place to update hint_message in this case, right? If it is,
it's not needed to use `+=`, and you can simplify it like
`response.hint_message = fmt::format("ERROR: app({}) not exist.",
old_app_name);`
The other places can be refactor in the same way.
btw, "check it!" is not necessary.
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,111 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ const auto &old_app_name = rpc.request().old_app_name;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ zauto_read_lock l;
+ auto target_app = get_app(old_app_name);
+
+ if (target_app == nullptr) {
+ auto &response = rpc.response();
+ std::ostringstream oss;
+ response.err = ERR_APP_NOT_EXIST;
+ oss << "ERROR: app(" << old_app_name << ") not exist. check it!" <<
std::endl;
+ response.hint_message += oss.str();
+ return;
+ }
+
+ LOG_INFO_F("ready to update remote app_name: app_id={}, old_app_name={},
new_app_name={}.",
+ target_app->app_id,
+ old_app_name,
+ new_app_name);
+
+ auto ainfo = *(reinterpret_cast<app_info *>(target_app.get()));
+ ainfo.app_name = new_app_name;
+ auto app_path = get_app_path(*target_app);
+
+ do_update_app_info(app_path, ainfo, [this, target_app, rpc](error_code ec)
mutable {
+ const auto &new_app_name = rpc.request().new_app_name;
+ zauto_write_lock l(_lock);
+
+ CHECK_EQ(ec, ERR_OK);
+
+ const auto old_app_name = target_app->app_name;
+ target_app->app_name = new_app_name;
+
+ _exist_apps.emplace(new_app_name, target_app);
+ _exist_apps.erase(old_app_name);
+
+ LOG_INFO_F("both remote and local env of app_name have been updated "
+ "successfully: app_id={}, old_app_name={}, new_app_name={}",
+ target_app->app_id,
+ old_app_name,
+ new_app_name);
+
+ auto &response = rpc.response();
+ response.err = ERR_OK;
+
+ });
+}
+
+void server_state::rename_app(configuration_rename_app_rpc rpc)
+{
+ const std::string &old_app_name = rpc.request().old_app_name;
+ const std::string &new_app_name = rpc.request().new_app_name;
+ auto &response = rpc.response();
+
+ std::shared_ptr<app_state> target_app;
+ bool do_rename = false;
+ LOG_INFO_F(
+ "rename app request, old_app_name({}), new_app_name({})",
old_app_name, new_app_name);
+
+ zauto_read_lock l(_lock);
+ target_app = get_app(old_app_name);
+
+ std::ostringstream oss;
+ if (target_app == nullptr) {
+ response.err = ERR_APP_NOT_EXIST;
+ oss << "ERROR: app(" << old_app_name << ") not exist. check it!" <<
std::endl;
+ response.hint_message += oss.str();
+ return;
+ }
+
+ switch (target_app->status) {
+ case app_status::AS_AVAILABLE: {
+ if (_exist_apps.find(new_app_name) != _exist_apps.end()) {
+ response.err = ERR_INVALID_PARAMETERS;
+ oss << "ERROR: app(" << new_app_name << ") already exist! check
it!" << std::endl;
+ response.hint_message += oss.str();
+ return;
+ }
+ do_rename = true;
Review Comment:
why not call `do_app_rename` directly here?
##########
src/meta/test/meta_app_operation_test.cpp:
##########
@@ -795,5 +809,28 @@ TEST_F(meta_app_operation_test,
recover_from_max_replica_count_env)
verify_app_max_replica_count(APP_NAME, new_max_replica_count);
}
+TEST_F(meta_app_operation_test, rename_app)
+{
+ const std::string app_name_1 = APP_NAME + "_rename_1";
+ create_app(app_name_1);
+ auto app = find_app(app_name_1);
+ auto app_id_1 = app->app_id;
+
+ const std::string app_name_2 = APP_NAME + "_rename_2";
+ create_app(app_name_2);
+ app = find_app(app_name_2);
+ auto app_id_2 = app->app_id;
+
+ const std::string app_name_3 = APP_NAME + "_rename_3";
+
+ auto resp = rename_app(app_name_1, app_name_2);
+ ASSERT_EQ(resp.err, ERR_INVALID_PARAMETERS);
Review Comment:
Check the hint message as well?
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,111 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ const auto &old_app_name = rpc.request().old_app_name;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ zauto_read_lock l;
+ auto target_app = get_app(old_app_name);
+
+ if (target_app == nullptr) {
+ auto &response = rpc.response();
+ std::ostringstream oss;
+ response.err = ERR_APP_NOT_EXIST;
+ oss << "ERROR: app(" << old_app_name << ") not exist. check it!" <<
std::endl;
+ response.hint_message += oss.str();
+ return;
+ }
+
+ LOG_INFO_F("ready to update remote app_name: app_id={}, old_app_name={},
new_app_name={}.",
+ target_app->app_id,
+ old_app_name,
+ new_app_name);
+
+ auto ainfo = *(reinterpret_cast<app_info *>(target_app.get()));
+ ainfo.app_name = new_app_name;
+ auto app_path = get_app_path(*target_app);
+
+ do_update_app_info(app_path, ainfo, [this, target_app, rpc](error_code ec)
mutable {
+ const auto &new_app_name = rpc.request().new_app_name;
+ zauto_write_lock l(_lock);
+
+ CHECK_EQ(ec, ERR_OK);
+
+ const auto old_app_name = target_app->app_name;
+ target_app->app_name = new_app_name;
+
+ _exist_apps.emplace(new_app_name, target_app);
+ _exist_apps.erase(old_app_name);
+
+ LOG_INFO_F("both remote and local env of app_name have been updated "
+ "successfully: app_id={}, old_app_name={}, new_app_name={}",
+ target_app->app_id,
+ old_app_name,
+ new_app_name);
+
+ auto &response = rpc.response();
+ response.err = ERR_OK;
Review Comment:
The default value is `ERR_OK` and you can remove the manully assign code?
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,111 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ const auto &old_app_name = rpc.request().old_app_name;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ zauto_read_lock l;
+ auto target_app = get_app(old_app_name);
+
+ if (target_app == nullptr) {
+ auto &response = rpc.response();
+ std::ostringstream oss;
+ response.err = ERR_APP_NOT_EXIST;
+ oss << "ERROR: app(" << old_app_name << ") not exist. check it!" <<
std::endl;
+ response.hint_message += oss.str();
+ return;
+ }
+
+ LOG_INFO_F("ready to update remote app_name: app_id={}, old_app_name={},
new_app_name={}.",
Review Comment:
can be removed.
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,111 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ const auto &old_app_name = rpc.request().old_app_name;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ zauto_read_lock l;
+ auto target_app = get_app(old_app_name);
+
+ if (target_app == nullptr) {
+ auto &response = rpc.response();
+ std::ostringstream oss;
+ response.err = ERR_APP_NOT_EXIST;
+ oss << "ERROR: app(" << old_app_name << ") not exist. check it!" <<
std::endl;
+ response.hint_message += oss.str();
+ return;
+ }
+
+ LOG_INFO_F("ready to update remote app_name: app_id={}, old_app_name={},
new_app_name={}.",
+ target_app->app_id,
+ old_app_name,
+ new_app_name);
+
+ auto ainfo = *(reinterpret_cast<app_info *>(target_app.get()));
+ ainfo.app_name = new_app_name;
+ auto app_path = get_app_path(*target_app);
+
+ do_update_app_info(app_path, ainfo, [this, target_app, rpc](error_code ec)
mutable {
+ const auto &new_app_name = rpc.request().new_app_name;
+ zauto_write_lock l(_lock);
+
+ CHECK_EQ(ec, ERR_OK);
+
+ const auto old_app_name = target_app->app_name;
+ target_app->app_name = new_app_name;
+
+ _exist_apps.emplace(new_app_name, target_app);
+ _exist_apps.erase(old_app_name);
+
+ LOG_INFO_F("both remote and local env of app_name have been updated "
+ "successfully: app_id={}, old_app_name={}, new_app_name={}",
+ target_app->app_id,
+ old_app_name,
+ new_app_name);
+
+ auto &response = rpc.response();
+ response.err = ERR_OK;
+
Review Comment:
remove the blank line.
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,111 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ const auto &old_app_name = rpc.request().old_app_name;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ zauto_read_lock l;
+ auto target_app = get_app(old_app_name);
+
+ if (target_app == nullptr) {
Review Comment:
target_app has been checked in `rename_app` under a lock, how can this
situaction happened?
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,111 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ const auto &old_app_name = rpc.request().old_app_name;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ zauto_read_lock l;
Review Comment:
what is it used for?
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,111 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
Review Comment:
How about rename to `rename_app_unlock`? the `_unlock` postfix is often used
for the functions which must hold a lock by caller.
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,111 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ const auto &old_app_name = rpc.request().old_app_name;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ zauto_read_lock l;
+ auto target_app = get_app(old_app_name);
+
+ if (target_app == nullptr) {
+ auto &response = rpc.response();
+ std::ostringstream oss;
+ response.err = ERR_APP_NOT_EXIST;
+ oss << "ERROR: app(" << old_app_name << ") not exist. check it!" <<
std::endl;
+ response.hint_message += oss.str();
+ return;
+ }
+
+ LOG_INFO_F("ready to update remote app_name: app_id={}, old_app_name={},
new_app_name={}.",
+ target_app->app_id,
+ old_app_name,
+ new_app_name);
+
+ auto ainfo = *(reinterpret_cast<app_info *>(target_app.get()));
+ ainfo.app_name = new_app_name;
+ auto app_path = get_app_path(*target_app);
+
+ do_update_app_info(app_path, ainfo, [this, target_app, rpc](error_code ec)
mutable {
+ const auto &new_app_name = rpc.request().new_app_name;
+ zauto_write_lock l(_lock);
+
+ CHECK_EQ(ec, ERR_OK);
+
+ const auto old_app_name = target_app->app_name;
+ target_app->app_name = new_app_name;
+
+ _exist_apps.emplace(new_app_name, target_app);
+ _exist_apps.erase(old_app_name);
+
+ LOG_INFO_F("both remote and local env of app_name have been updated "
+ "successfully: app_id={}, old_app_name={}, new_app_name={}",
+ target_app->app_id,
+ old_app_name,
+ new_app_name);
+
+ auto &response = rpc.response();
+ response.err = ERR_OK;
+
+ });
+}
+
+void server_state::rename_app(configuration_rename_app_rpc rpc)
+{
+ const std::string &old_app_name = rpc.request().old_app_name;
+ const std::string &new_app_name = rpc.request().new_app_name;
+ auto &response = rpc.response();
+
+ std::shared_ptr<app_state> target_app;
+ bool do_rename = false;
+ LOG_INFO_F(
+ "rename app request, old_app_name({}), new_app_name({})",
old_app_name, new_app_name);
Review Comment:
It is recommand to delcare variables closer to the first time used place. So
the code can be refactored like:
```
const std::string &old_app_name = rpc.request().old_app_name;
const std::string &new_app_name = rpc.request().new_app_name;
LOG_INFO_F(
"rename app request, old_app_name({}), new_app_name({})", old_app_name,
new_app_name);
zauto_read_lock l(_lock);
auto target_app = get_app(old_app_name);
....
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
