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/01 17:12:21 UTC
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1272: feat: add rename_app interface
acelyc111 commented on code in PR #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272#discussion_r1037347165
##########
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", "<app_id> [new_app_name]", rename_app,
Review Comment:
IMO, app name would be better than app id, it's different from recall which operate on tables are hiden.
##########
src/shell/commands/table_management.cpp:
##########
@@ -721,6 +721,40 @@ bool drop_app(command_executor *e, shell_context *sc, arguments args)
return true;
}
+bool rename_app(command_executor *e, shell_context *sc, arguments args)
+{
+ if (args.argc <= 2) {
+ return false;
+ }
+
+ int id;
+ if (!dsn::buf2int32(args.argv[1], id)) {
+ fprintf(stderr, "ERROR: parse %s as id failed\n", args.argv[1]);
+ return false;
+ }
+ std::string new_name = args.argv[2];
+
+ auto err_resp = sc->ddl_client->rename_app(id, new_name);
+ auto err = err_resp.get_error();
+ const auto &resp = err_resp.get_value();
+
+ if (dsn_likely(err.is_ok())) {
+ err = dsn::error_s::make(resp.err);
+ }
+
+ if (err.is_ok()) {
+ fmt::print(stdout, "rename app ok, app_id({}), new_app_name({})\n", id, new_name);
+ } else {
+ fmt::print(stderr,
+ "rename app failed, app_id({}), new_app_name({}), failed: {}\n",
+ id,
+ new_name,
+ resp.err.to_string());
Review Comment:
```suggestion
resp.err);
```
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,90 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ int32_t app_id = rpc.request().app_id;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ auto target_app = get_app(app_id);
+
+ LOG_INFO_F("ready to update remote app_name: app_id={}, app_name={}",
+ target_app->app_id,
+ 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(ec == ERR_OK,
Review Comment:
CHECK_EQ
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,90 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ int32_t app_id = rpc.request().app_id;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ auto target_app = get_app(app_id);
+
+ LOG_INFO_F("ready to update remote app_name: app_id={}, app_name={}",
Review Comment:
show the old name in logs
##########
src/common/meta_admin.thrift:
##########
@@ -161,6 +161,17 @@ struct configuration_drop_app_response
1:dsn.error_code err;
}
+struct configuration_rename_app_request
+{
+ 1:i32 app_id;
+ 2:string new_app_name;
+}
+
+struct configuration_rename_app_response
+{
+ 1:dsn.error_code err;
Review Comment:
Could you fill some hint message ?
##########
src/shell/commands/table_management.cpp:
##########
@@ -721,6 +721,40 @@ bool drop_app(command_executor *e, shell_context *sc, arguments args)
return true;
}
+bool rename_app(command_executor *e, shell_context *sc, arguments args)
+{
+ if (args.argc <= 2) {
+ return false;
+ }
+
+ int id;
+ if (!dsn::buf2int32(args.argv[1], id)) {
+ fprintf(stderr, "ERROR: parse %s as id failed\n", args.argv[1]);
Review Comment:
also use fmt
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,90 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ int32_t app_id = rpc.request().app_id;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ auto target_app = get_app(app_id);
Review Comment:
deal when app not found
##########
src/replica/replica_config.cpp:
##########
@@ -1070,6 +1071,26 @@ void replica::on_config_sync(const app_info &info,
}
}
+void replica::update_app_name(const std::string app_name)
+{
+ if (app_name == _app_info.app_name) {
+ return;
+ }
+
+ auto old_app_name = _app_info.app_name;
+ _app_info.app_name = app_name;
+
+ auto ec = store_app_info(_app_info);
+ CHECK_EQ_PREFIX_MSG(ec,
+ ERR_OK,
+ "store_app_info for app_name failed: error_code={}, "
+ "app_name={}, app_id={}, old_app_name={}",
+ ec.to_string(),
Review Comment:
`ec` will be print automatically when use CHECK_EQ_PREFIX_MSG, it's not needed to print it again.
##########
src/client/replication_ddl_client.cpp:
##########
@@ -1738,5 +1738,19 @@ replication_ddl_client::set_max_replica_count(const std::string &app_name,
configuration_set_max_replica_count_rpc(std::move(req), RPC_CM_SET_MAX_REPLICA_COUNT));
}
+error_with<configuration_rename_app_response>
+replication_ddl_client::rename_app(int32_t app_id, const std::string &new_app_name)
+{
+ if (!std::all_of(new_app_name.cbegin(),
+ new_app_name.cend(),
+ (bool (*)(int))replication_ddl_client::valid_app_char)) {
+ return FMT_ERR(ERR_INVALID_PARAMETERS, "not support format for new_app_name");
Review Comment:
Could you also remind which characters are valid in return message?
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,90 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ int32_t app_id = rpc.request().app_id;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ auto target_app = get_app(app_id);
+
+ LOG_INFO_F("ready to update remote app_name: app_id={}, app_name={}",
+ target_app->app_id,
+ 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(ec == ERR_OK,
+ "An error that can't be handled occurs while updating remote app app_name "
+ "app_name: error_code={}, app_id={}, app_name={}, new_app_name={}",
+ ec.to_string(),
+ target_app->app_id,
+ target_app->app_name,
+ new_app_name);
+
+ 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={}, new_app_name={}",
+ target_app->app_id,
+ target_app->app_name);
+
+ auto &response = rpc.response();
+ response.err = ERR_OK;
+
+ });
+}
+
+void server_state::rename_app(configuration_rename_app_rpc rpc)
+{
+ int32_t app_id = rpc.request().app_id;
+ 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, app_id({}), new_app_name({})", app_id, new_app_name);
+
+ {
+ zauto_read_lock l(_lock);
+ target_app = get_app(app_id);
+ if (target_app == nullptr) {
+ response.err = ERR_APP_NOT_EXIST;
+ } else if (target_app->status != app_status::AS_AVAILABLE) {
+ if (target_app->status == app_status::AS_CREATING ||
+ target_app->status == app_status::AS_RECALLING) {
+ response.err = ERR_BUSY_CREATING;
+ } else if (target_app->status == app_status::AS_DROPPING) {
+ response.err = ERR_BUSY_DROPPING;
+ } else if (target_app->status == app_status::AS_DROPPED) {
+ response.err = ERR_APP_DROPPED;
+ } else {
+ // AS_INVALID/AS_CREATE_FAILED/AS_DROP_FAILED
+ response.err = ERR_INVALID_STATE;
+ }
+ } else {
+ if (_exist_apps.find(new_app_name) != _exist_apps.end()) {
+ response.err = ERR_INVALID_PARAMETERS;
+ } else {
+ do_rename = true;
+ }
+ }
+ }
+
+ if (do_rename) {
+ do_app_rename(rpc);
Review Comment:
it should be in lock scope as well
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,90 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ int32_t app_id = rpc.request().app_id;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ auto target_app = get_app(app_id);
+
+ LOG_INFO_F("ready to update remote app_name: app_id={}, app_name={}",
+ target_app->app_id,
+ 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(ec == ERR_OK,
+ "An error that can't be handled occurs while updating remote app app_name "
+ "app_name: error_code={}, app_id={}, app_name={}, new_app_name={}",
+ ec.to_string(),
Review Comment:
no need to print ec
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,90 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ int32_t app_id = rpc.request().app_id;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ auto target_app = get_app(app_id);
+
+ LOG_INFO_F("ready to update remote app_name: app_id={}, app_name={}",
+ target_app->app_id,
+ 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(ec == ERR_OK,
+ "An error that can't be handled occurs while updating remote app app_name "
+ "app_name: error_code={}, app_id={}, app_name={}, new_app_name={}",
+ ec.to_string(),
+ target_app->app_id,
+ target_app->app_name,
+ new_app_name);
+
+ 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={}, new_app_name={}",
+ target_app->app_id,
+ target_app->app_name);
+
+ auto &response = rpc.response();
+ response.err = ERR_OK;
+
+ });
+}
+
+void server_state::rename_app(configuration_rename_app_rpc rpc)
+{
+ int32_t app_id = rpc.request().app_id;
+ 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, app_id({}), new_app_name({})", app_id, new_app_name);
+
+ {
+ zauto_read_lock l(_lock);
+ target_app = get_app(app_id);
+ if (target_app == nullptr) {
+ response.err = ERR_APP_NOT_EXIST;
+ } else if (target_app->status != app_status::AS_AVAILABLE) {
+ if (target_app->status == app_status::AS_CREATING ||
+ target_app->status == app_status::AS_RECALLING) {
+ response.err = ERR_BUSY_CREATING;
+ } else if (target_app->status == app_status::AS_DROPPING) {
+ response.err = ERR_BUSY_DROPPING;
+ } else if (target_app->status == app_status::AS_DROPPED) {
+ response.err = ERR_APP_DROPPED;
+ } else {
+ // AS_INVALID/AS_CREATE_FAILED/AS_DROP_FAILED
+ response.err = ERR_INVALID_STATE;
+ }
+ } else {
+ if (_exist_apps.find(new_app_name) != _exist_apps.end()) {
+ response.err = ERR_INVALID_PARAMETERS;
+ } else {
+ do_rename = true;
Review Comment:
reduce if-else statements by:
```
do {
if cond1
...
break;
if cond2
...
break;
...
} while (false);
```
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,90 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ int32_t app_id = rpc.request().app_id;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ auto target_app = get_app(app_id);
+
+ LOG_INFO_F("ready to update remote app_name: app_id={}, app_name={}",
+ target_app->app_id,
+ 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(ec == ERR_OK,
+ "An error that can't be handled occurs while updating remote app app_name "
+ "app_name: error_code={}, app_id={}, app_name={}, new_app_name={}",
+ ec.to_string(),
+ target_app->app_id,
+ target_app->app_name,
+ new_app_name);
+
+ 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={}, new_app_name={}",
+ target_app->app_id,
+ target_app->app_name);
+
+ auto &response = rpc.response();
+ response.err = ERR_OK;
+
+ });
+}
+
+void server_state::rename_app(configuration_rename_app_rpc rpc)
+{
+ int32_t app_id = rpc.request().app_id;
+ 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, app_id({}), new_app_name({})", app_id, new_app_name);
+
+ {
+ zauto_read_lock l(_lock);
+ target_app = get_app(app_id);
+ if (target_app == nullptr) {
+ response.err = ERR_APP_NOT_EXIST;
+ } else if (target_app->status != app_status::AS_AVAILABLE) {
+ if (target_app->status == app_status::AS_CREATING ||
+ target_app->status == app_status::AS_RECALLING) {
+ response.err = ERR_BUSY_CREATING;
+ } else if (target_app->status == app_status::AS_DROPPING) {
+ response.err = ERR_BUSY_DROPPING;
+ } else if (target_app->status == app_status::AS_DROPPED) {
+ response.err = ERR_APP_DROPPED;
+ } else {
+ // AS_INVALID/AS_CREATE_FAILED/AS_DROP_FAILED
+ response.err = ERR_INVALID_STATE;
+ }
Review Comment:
how about use switch..case.. ?
--
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