You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by GitBox <gi...@apache.org> on 2022/12/05 16:27:14 UTC
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1272: feat: add rename_app interface
empiredan commented on code in PR #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272#discussion_r1039575429
##########
src/shell/main.cpp:
##########
@@ -91,6 +91,9 @@ static command_executor commands[] = {
{
"recall", "recall an app", "<app_id> [new_app_name]", recall_app,
},
+ {
+ "rename", "rename an app", "<old_app_name> [new_app_name]", rename_app,
Review Comment:
The argument `new_app_name` is required:
```suggestion
"rename", "rename an app", "<old_app_name> <new_app_name>", rename_app,
```
##########
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;
Review Comment:
```suggestion
const auto &old_app_name = rpc.request().old_app_name;
```
##########
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;
Review Comment:
```suggestion
const auto &new_app_name = rpc.request().new_app_name;
```
##########
src/client/replication_ddl_client.cpp:
##########
@@ -53,6 +53,13 @@
namespace dsn {
namespace replication {
+#define VALIDATE_TABLE_NAME(app_name) \
+ if (app_name.empty() || \
+ !std::all_of(app_name.cbegin(), \
+ app_name.cend(), \
+ (bool (*)(int))replication_ddl_client::valid_app_char)) \
+ return FMT_ERR(ERR_INVALID_PARAMETERS, "Invalid name. Only 0-9a-zA-Z.:_ are valid!");
Review Comment:
Can be surrounded by:
```c++
do {
...
} while (false)
```
##########
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);
Review Comment:
Print more information to help find problems, such as table id and name ?
##########
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;
Review Comment:
Should we check again whether the new table name has been used, since this is called asynchronously, maybe a new table named `new_app_name` has been created successfully ?
##########
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)
+{
Review Comment:
Some problems:
1. Can add some comments to show which cases are tested here;
2. Add more cases to cover, for example:
* if the new app name has invalid character ?
* if the old app does not existed ?
##########
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:
This line should be dropped:
```suggestion
```
##########
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:
```suggestion
```
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org