[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1188: feat: add rename_app interface

[GitBox <gi...@apache.org>]

acelyc111 commented on code in PR #1188:
URL: https://github.com/apache/incubator-pegasus/pull/1188#discussion_r997709465


##########
src/rdsn/src/client/replication_ddl_client.cpp:
##########
@@ -215,6 +215,33 @@ dsn::error_code replication_ddl_client::drop_app(const std::string &app_name, in
     return dsn::ERR_OK;
 }
 
+dsn::error_code 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 ERR_INVALID_PARAMETERS;
+
+    std::shared_ptr<configuration_rename_app_request> req =
+        std::make_shared<configuration_rename_app_request>();

Review Comment:
   ```suggestion
       auto req = std::make_shared<configuration_rename_app_request>();
   ```



##########
src/shell/commands/table_management.cpp:
##########
@@ -721,6 +721,26 @@ 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];
+
+    ::dsn::error_code err = sc->ddl_client->rename_app(id, new_name);
+    if (dsn::ERR_OK == err)
+        std::cout << "rename app " << id << " succeed" << std::endl;
+    else
+        std::cout << "rename app " << id << " failed, error=" << err.to_string() << std::endl;

Review Comment:
   Same



##########
src/rdsn/src/replica/replica_stub.cpp:
##########
@@ -1346,6 +1346,41 @@ void replica_stub::get_local_replicas(std::vector<replica_info> &replicas)
     }
 }
 
+void replica_stub::on_update_local_configuration(update_node_configuration_rpc rpc)
+{
+    const update_node_configuration_request &req = rpc.request();
+    update_node_configuration_response &resp = rpc.response();

Review Comment:
   how about use `auto`?



##########
src/rdsn/src/replica/replica_stub.cpp:
##########
@@ -1346,6 +1346,41 @@ void replica_stub::get_local_replicas(std::vector<replica_info> &replicas)
     }
 }
 
+void replica_stub::on_update_local_configuration(update_node_configuration_rpc rpc)
+{
+    const update_node_configuration_request &req = rpc.request();
+    update_node_configuration_response &resp = rpc.response();
+    if (_state == NS_Disconnected || _config_query_task != nullptr) {
+        resp.err = ERR_INVALID_STATE;
+        return;
+    }
+
+    dsn::message_ex *msg = dsn::message_ex::create_request(RPC_CM_CONFIG_SYNC);
+
+    configuration_query_by_node_request req;
+    req.node = _primary_address;
+
+    // TODO: send stored replicas may cost network, we shouldn't config the frequency
+    get_local_replicas(req.stored_replicas);
+    req.__isset.stored_replicas = true;
+
+    ::dsn::marshall(msg, req);
+
+    ddebug("send query node partitions request to meta server, stored_replicas_count = %d",

Review Comment:
   use ddebug_f



##########
src/rdsn/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(std::string app_name);

Review Comment:
   use const reference



##########
src/shell/commands/table_management.cpp:
##########
@@ -721,6 +721,26 @@ 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;

Review Comment:
   It's recommend to add braces even if there ony 1 line.



##########
src/rdsn/src/client/replication_ddl_client.cpp:
##########
@@ -215,6 +215,33 @@ dsn::error_code replication_ddl_client::drop_app(const std::string &app_name, in
     return dsn::ERR_OK;
 }
 
+dsn::error_code 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 ERR_INVALID_PARAMETERS;

Review Comment:
   Could you refactor it to a function, I saw this is the 3rd times it occured.



-- 
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