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/11/30 11:07:52 UTC
[GitHub] [incubator-pegasus] GehaFearless opened a new pull request, #1272: feat: add rename_app interface
GehaFearless opened a new pull request, #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272
issue: https://github.com/apache/incubator-pegasus/issues/1185
I want add a couple rpc to solve rename app.
--
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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272#discussion_r1045720855
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,87 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::rename_app(configuration_rename_app_rpc rpc)
+{
+ auto &response = rpc.response();
+ bool do_rename = false;
+
+ const auto &old_app_name = rpc.request().old_app_name;
+ const auto &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_write_lock l(_lock);
+ auto target_app = get_app(old_app_name);
+
+ if (target_app == nullptr) {
+ response.err = ERR_APP_NOT_EXIST;
+ response.hint_message = fmt::format("ERROR: app({}) not exist. check it!", old_app_name);
Review Comment:
```suggestion
response.hint_message = fmt::format("ERROR: app({}) not exist.", old_app_name);
```
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,87 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::rename_app(configuration_rename_app_rpc rpc)
+{
+ auto &response = rpc.response();
+ bool do_rename = false;
+
+ const auto &old_app_name = rpc.request().old_app_name;
+ const auto &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_write_lock l(_lock);
+ auto target_app = get_app(old_app_name);
+
+ if (target_app == nullptr) {
+ response.err = ERR_APP_NOT_EXIST;
+ response.hint_message = fmt::format("ERROR: app({}) not exist. check it!", old_app_name);
+ 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;
+ response.hint_message =
+ fmt::format("ERROR: app({}) already exist! check it!", old_app_name);
Review Comment:
```suggestion
fmt::format("ERROR: app({}) already exist!", new_app_name);
```
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,87 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::rename_app(configuration_rename_app_rpc rpc)
+{
+ auto &response = rpc.response();
+ bool do_rename = false;
+
+ const auto &old_app_name = rpc.request().old_app_name;
+ const auto &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_write_lock l(_lock);
+ auto target_app = get_app(old_app_name);
+
+ if (target_app == nullptr) {
+ response.err = ERR_APP_NOT_EXIST;
+ response.hint_message = fmt::format("ERROR: app({}) not exist. check it!", old_app_name);
+ 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;
+ response.hint_message =
+ fmt::format("ERROR: app({}) already exist! check it!", old_app_name);
+ return;
+ }
+ do_rename = true;
+ } break;
+ case app_status::AS_CREATING:
+ case app_status::AS_RECALLING: {
+ response.err = ERR_BUSY_CREATING;
+ } break;
+ case app_status::AS_DROPPING: {
+ response.err = ERR_BUSY_DROPPING;
+ } break;
+ case app_status::AS_DROPPED: {
+ response.err = ERR_APP_DROPPED;
+ } break;
+ default: {
+ response.err = ERR_INVALID_STATE;
+ } break;
+ }
+
+ if (!do_rename) {
+ response.hint_message =
+ fmt::format("ERROR: app({}) status can't execute rename.", old_app_name);
+ return;
+ }
+
+ auto app_id = target_app->app_id;
+
+ auto ainfo = *(reinterpret_cast<app_info *>(target_app.get()));
+ ainfo.app_name = new_app_name;
+ auto app_path = get_app_path(*target_app);
+
+ target_app->app_name = new_app_name;
+ _exist_apps.emplace(new_app_name, target_app);
+
+ do_update_app_info(app_path, ainfo, [this, app_id, rpc](error_code ec) mutable {
+ const auto &new_app_name = rpc.request().new_app_name;
+ const auto &old_app_name = rpc.request().old_app_name;
Review Comment:
how about pass them though?
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,87 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::rename_app(configuration_rename_app_rpc rpc)
+{
+ auto &response = rpc.response();
+ bool do_rename = false;
+
+ const auto &old_app_name = rpc.request().old_app_name;
+ const auto &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_write_lock l(_lock);
+ auto target_app = get_app(old_app_name);
+
+ if (target_app == nullptr) {
+ response.err = ERR_APP_NOT_EXIST;
+ response.hint_message = fmt::format("ERROR: app({}) not exist. check it!", old_app_name);
+ 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;
+ response.hint_message =
+ fmt::format("ERROR: app({}) already exist! check it!", old_app_name);
+ return;
+ }
+ do_rename = true;
+ } break;
+ case app_status::AS_CREATING:
+ case app_status::AS_RECALLING: {
+ response.err = ERR_BUSY_CREATING;
+ } break;
+ case app_status::AS_DROPPING: {
+ response.err = ERR_BUSY_DROPPING;
+ } break;
+ case app_status::AS_DROPPED: {
+ response.err = ERR_APP_DROPPED;
+ } break;
+ default: {
+ response.err = ERR_INVALID_STATE;
+ } break;
+ }
+
+ if (!do_rename) {
+ response.hint_message =
+ fmt::format("ERROR: app({}) status can't execute rename.", old_app_name);
+ return;
+ }
+
+ auto app_id = target_app->app_id;
+
+ auto ainfo = *(reinterpret_cast<app_info *>(target_app.get()));
+ ainfo.app_name = new_app_name;
+ auto app_path = get_app_path(*target_app);
+
+ target_app->app_name = new_app_name;
+ _exist_apps.emplace(new_app_name, target_app);
+
+ do_update_app_info(app_path, ainfo, [this, app_id, rpc](error_code ec) mutable {
+ const auto &new_app_name = rpc.request().new_app_name;
+ const auto &old_app_name = rpc.request().old_app_name;
+ zauto_write_lock l(_lock);
+
+ CHECK_EQ_MSG(ec,
+ ERR_OK,
+ "update remote app info failed: app_id={}, old_app_name={}, new_app_name={}",
+ app_id,
+ old_app_name,
+ new_app_name);
Review Comment:
```suggestion
CHECK_EQ_MSG(ec,
ERR_OK,
"update remote app info failed: app_id={}, old_app_name={}, new_app_name={}",
app_id,
old_app_name,
new_app_name);
zauto_write_lock l(_lock);
```
--
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
[GitHub] [incubator-pegasus] GehaFearless commented on a diff in pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
GehaFearless commented on code in PR #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272#discussion_r1044206937
##########
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:
`CHECK` and add some message is better.
--
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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272#discussion_r1045532212
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,102 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::rename_app_unlock(configuration_rename_app_rpc rpc)
+{
+ zauto_write_lock l(_lock);
+
+ const auto &old_app_name = rpc.request().old_app_name;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ auto target_app = get_app(old_app_name);
+ auto app_id = target_app->app_id;
+
+ auto ainfo = *(reinterpret_cast<app_info *>(target_app.get()));
+ ainfo.app_name = new_app_name;
+ auto app_path = get_app_path(*target_app);
+
+ target_app->app_name = new_app_name;
+ _exist_apps.emplace(new_app_name, target_app);
+
+ do_update_app_info(app_path, ainfo, [this, app_id, rpc](error_code ec) mutable {
+ const auto &new_app_name = rpc.request().new_app_name;
+ const auto &old_app_name = rpc.request().old_app_name;
+ zauto_write_lock l(_lock);
+
+ if (ERR_OK != ec) {
+ _exist_apps.erase(new_app_name);
+ CHECK(false,
Review Comment:
This is will cause the process crash, you can use `CHECK_EQ(ERR_OK, ...)` as before, `_exist_apps.erase(new_app_name);` is meanless because the process will crash soon.
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,102 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::rename_app_unlock(configuration_rename_app_rpc rpc)
+{
+ zauto_write_lock l(_lock);
Review Comment:
Since the function requires the lock itself, the function name should be `do_rename_app` (or sth like that).
Further more, you can remove the lock if you pass `rpc`, `app_id`, and call `_exist_apps.emplace(...)` in rename_app?
--
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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272#discussion_r1044311914
##########
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:
In gtest CHECK will cause the process crash and the test will terminate, but ASSERT_* will not.
You can add message like `ASSERT_TRUE(app) << fmt:format("app({}) does not exist", app_name_1);` too.
--
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
[GitHub] [incubator-pegasus] GehaFearless commented on a diff in pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
GehaFearless commented on code in PR #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272#discussion_r1044175644
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,105 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ zauto_write_lock l(_lock);
+
+ const auto &old_app_name = rpc.request().old_app_name;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ auto target_app = get_app(old_app_name);
+ auto app_id = target_app->app_id;
+
+ auto ainfo = *(reinterpret_cast<app_info *>(target_app.get()));
+ ainfo.app_name = new_app_name;
+ auto app_path = get_app_path(*target_app);
+
+ target_app->app_name = new_app_name;
+ _exist_apps.emplace(new_app_name, target_app);
Review Comment:
`do_update_app_info` is a async call. Maybe a new table named new_app_name has been created successfully，So add it firstly.
--
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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
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: 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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272#discussion_r1044311914
##########
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:
In gtest CHECK will cause the process crash and no message left, but ASSERT_* will not.
--
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
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272#discussion_r1038988788
##########
src/replica/replica_config.cpp:
##########
@@ -1070,6 +1071,25 @@ 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,
Review Comment:
```suggestion
CHECK_EQ_PREFIX_MSG(store_app_info(_app_info),
```
##########
src/client/replication_ddl_client.cpp:
##########
@@ -1738,5 +1738,27 @@ 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(const std::string &old_app_name, const std::string &new_app_name)
+{
+ if (old_app_name.empty() ||
+ !std::all_of(old_app_name.cbegin(),
+ old_app_name.cend(),
+ (bool (*)(int))replication_ddl_client::valid_app_char))
+ return FMT_ERR(ERR_INVALID_PARAMETERS, "invalid old_app_name.");
Review Comment:
It seems this code is duplicated some times, could you add a macros like:
```
VALIDATE_TABLE_NAME(table_name, msg) \
if (table_name.empty() || ...) { \
return FMT_ERR(ERR_INVALID_PARAMETERS, #msg);
```
you can do it in another patch and refactor some old code.
##########
src/replica/replica.h:
##########
@@ -496,6 +496,7 @@ class replica : public serverlet<replica>, public ref_counter, public replica_ba
clear_on_failure(replica_stub *stub, replica *rep, const std::string &path, const gpid &pid);
void update_app_max_replica_count(int32_t max_replica_count);
+ void update_app_name(const std::string app_name);
Review Comment:
```suggestion
void update_app_name(const std::string &app_name);
```
##########
src/replica/replica_config.cpp:
##########
@@ -1070,6 +1071,25 @@ void replica::on_config_sync(const app_info &info,
}
}
+void replica::update_app_name(const std::string app_name)
Review Comment:
```suggestion
void replica::update_app_name(const std::string &app_name)
```
##########
src/client/replication_ddl_client.cpp:
##########
@@ -1738,5 +1738,27 @@ 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(const std::string &old_app_name, const std::string &new_app_name)
+{
+ if (old_app_name.empty() ||
+ !std::all_of(old_app_name.cbegin(),
+ old_app_name.cend(),
+ (bool (*)(int))replication_ddl_client::valid_app_char))
+ return FMT_ERR(ERR_INVALID_PARAMETERS, "invalid old_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. You can use "
+ "letters and numbers, and also char in ['.', ':', "
+ "'_'].");
Review Comment:
How about simplified by "Only 0-9a-zA-Z.:_ are valid" ? You can append it to the message in the macro mentioned above.
--
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
[GitHub] [incubator-pegasus] levy5307 commented on a diff in pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
levy5307 commented on code in PR #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272#discussion_r1048359310
##########
src/client/replication_ddl_client.cpp:
##########
@@ -53,6 +53,15 @@
namespace dsn {
namespace replication {
+#define VALIDATE_TABLE_NAME(app_name) \
Review Comment:
```
inline bool validate_table_name(std::string app_name) {
...
}
```
I think it's better to implement it by inline func
--
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
[GitHub] [incubator-pegasus] GehaFearless commented on a diff in pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
GehaFearless commented on code in PR #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272#discussion_r1044182745
##########
src/meta/test/meta_app_operation_test.cpp:
##########
@@ -795,5 +809,37 @@ 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);
+ CHECK(app, "app({}) does not exist", 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);
+ CHECK(app, "app({}) does not exist", app_name_2);
+ auto app_id_2 = app->app_id;
+
+ const std::string app_name_3 = APP_NAME + "_rename_3";
+
+ // case 1: new_app_name table exist
+ auto resp = rename_app(app_name_1, app_name_2);
+ ASSERT_EQ(ERR_INVALID_PARAMETERS, resp.err);
+
Review Comment:
Valid character checking in `ddl_client`, server not check it. So I think could ignore it here.
--
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
[GitHub] [incubator-pegasus] GehaFearless commented on a diff in pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
GehaFearless commented on code in PR #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272#discussion_r1044175644
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,105 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ zauto_write_lock l(_lock);
+
+ const auto &old_app_name = rpc.request().old_app_name;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ auto target_app = get_app(old_app_name);
+ auto app_id = target_app->app_id;
+
+ auto ainfo = *(reinterpret_cast<app_info *>(target_app.get()));
+ ainfo.app_name = new_app_name;
+ auto app_path = get_app_path(*target_app);
+
+ target_app->app_name = new_app_name;
+ _exist_apps.emplace(new_app_name, target_app);
Review Comment:
Maybe a new table named new_app_name has been created successfully，So add it firstly.
--
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
[GitHub] [incubator-pegasus] GehaFearless commented on a diff in pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
GehaFearless commented on code in PR #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272#discussion_r1045576718
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,102 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::rename_app_unlock(configuration_rename_app_rpc rpc)
+{
+ zauto_write_lock l(_lock);
Review Comment:
I see. I think merge two functions into one is better.
--
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
[GitHub] [incubator-pegasus] GehaFearless commented on a diff in pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
GehaFearless commented on code in PR #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272#discussion_r1040563847
##########
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:
thx
--
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
[GitHub] [incubator-pegasus] GehaFearless commented on a diff in pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
GehaFearless commented on code in PR #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272#discussion_r1040569613
##########
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:
For a precise function，also keep style with others and be good for transformation and optimization.
--
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
[GitHub] [incubator-pegasus] acelyc111 merged pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
acelyc111 merged PR #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272
--
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
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1272: feat: add rename_app interface
Posted by GitBox <gi...@apache.org>.
empiredan commented on code in PR #1272:
URL: https://github.com/apache/incubator-pegasus/pull/1272#discussion_r1042888650
##########
src/meta/test/meta_app_operation_test.cpp:
##########
@@ -795,5 +809,37 @@ 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);
+ CHECK(app, "app({}) does not exist", 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);
+ CHECK(app, "app({}) does not exist", app_name_2);
+ auto app_id_2 = app->app_id;
+
+ const std::string app_name_3 = APP_NAME + "_rename_3";
+
+ // case 1: new_app_name table exist
+ auto resp = rename_app(app_name_1, app_name_2);
+ ASSERT_EQ(ERR_INVALID_PARAMETERS, resp.err);
+
Review Comment:
Could add a case that if the new app name has invalid character.
##########
src/meta/server_state.cpp:
##########
@@ -1259,6 +1259,105 @@ void server_state::drop_app(dsn::message_ex *msg)
}
}
+void server_state::do_app_rename(configuration_rename_app_rpc rpc)
+{
+ zauto_write_lock l(_lock);
+
+ const auto &old_app_name = rpc.request().old_app_name;
+ const auto &new_app_name = rpc.request().new_app_name;
+
+ auto target_app = get_app(old_app_name);
+ auto app_id = target_app->app_id;
+
+ auto ainfo = *(reinterpret_cast<app_info *>(target_app.get()));
+ ainfo.app_name = new_app_name;
+ auto app_path = get_app_path(*target_app);
+
+ target_app->app_name = new_app_name;
+ _exist_apps.emplace(new_app_name, target_app);
Review Comment:
Why not add new app name after updating remote storage ? That will be an atomic operation including both adding new table name and removing old table 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: 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