Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

cli: refactor cli related code #156

Merged
merged 3 commits into from
Sep 13, 2018
Merged

Conversation

shengofsun
Copy link
Contributor

@shengofsun shengofsun commented Sep 9, 2018

这个pr主要干的事情:

  1. 以前cli是集成在core里面的。但这只是一个简单的rpc service,所以直接挪出来了。这样配置和core层的代码都好理解一些。
  2. 以前command manager和rpc的处理是耦合的。现在解耦了,这样用别的方式来调用这些command也容易很多。(比如可以加个http的?)
  3. cli的头文件和实现以前散落在不同地方,现在统一到了dist下。特别的,src/tools下面cli的一个小shell也放回到和cli模块一起。
  4. 把cli_remote/cli_local这两个配置删了,相应的配置以后在新版本集群配置里也没必要了。

另外:

  1. src/tools这个目录只剩下没用的repli了,这个pr就不动这个地方了。谁有空可以单独提个pr删一下。

bin/dsn.cmake Outdated
@@ -413,6 +413,7 @@ function(dsn_setup_compiler_flags)
add_compile_options(-Wno-unused-variable)
add_compile_options(-Wno-deprecated-declarations)
add_compile_options(-Wno-inconsistent-missing-override)
add_compile_options(-Wno-attributes)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个attribute是s2编译需要的。不加的话gcc7编译不过

@neverchanje
Copy link
Contributor

neverchanje commented Sep 9, 2018

really nice job! but there're still something i doubt that need to fix:

first, since the _http_server is placed in meta_service_app, could the cli_service be placed there too? could you adapt the code style with the _http_server so we can make further refactoring easier?

second, why the cli_service have to call open_service after its constructor, like:

    // start cli service before acquiring leader lock,
    // so that the command line call can be handled
    _cli_service = std::move(dsn::cli_service::create_service());
    _cli_service->open_service();

why not just:

: _cli_service(new cli_service())

and in the constructor you can "open the service" and don't have to split up the logic into two different functions:

cli_service() : ::dsn::serverlet<cli_service>("cli") 
{
  register_async_rpc_handler(RPC_CLI_CLI_CALL, "call", &cli_service::on_call);
}

~cli_service() 
{
  unregister_rpc_handler(RPC_CLI_CLI_CALL);
}

@@ -1,4 +1,4 @@
set(MY_PROJ_NAME dsn.cli.tool)
set(MY_PROJ_NAME cli_sh)
Copy link
Contributor

@neverchanje neverchanje Sep 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call this dsn_cli_shell?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个晚上我抽空改一下吧,白天没时间……

@shengofsun
Copy link
Contributor Author

@neverchanje

  1. "open_service" function for "xxx_service" is generated by the code generator, here we just keep the same with old conventions. We can modify this in a seperated pr after the code generator is refactored.
  2. “putting the cli_service into meta_service and replica_stub” follows the old conventions, too. As all the services are handled by replica_stub & meta_service, only the "http_service" is an exception:-(

@neverchanje
Copy link
Contributor

neverchanje commented Sep 10, 2018

ok... and i will move http_service to meta_service after this committed.

1. move cli to module dist
2. remove rpc related source code in command_manager, decouple
   cli command execution and command handler management.
3. move cli shell from tool to dist
@shengofsun
Copy link
Contributor Author

@ALL has fixed based on comments

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants