From e2cbb602da26e3d08202aabc48db628da52c45e6 Mon Sep 17 00:00:00 2001 From: neverchanje Date: Fri, 2 Apr 2021 15:22:15 +0800 Subject: [PATCH 1/3] refactor: refine help command --- src/runtime/profiler.cpp | 10 +-- src/utils/command_manager.cpp | 90 ++++--------------- ...d_manager.cpp => command_manager_test.cpp} | 59 ++++++------ src/utils/test/main.cpp | 1 - 4 files changed, 53 insertions(+), 107 deletions(-) rename src/utils/test/{command_manager.cpp => command_manager_test.cpp} (67%) diff --git a/src/runtime/profiler.cpp b/src/runtime/profiler.cpp index b7f2e303c5..d5612cfdf4 100644 --- a/src/runtime/profiler.cpp +++ b/src/runtime/profiler.cpp @@ -394,18 +394,18 @@ void register_command_profiler() textpd << textarg.str(); textquery << textarg.str(); - command_manager::instance().register_command({"p", "P", "profile", "Profile"}, - "profile|Profile|p|P - performance profiling", + command_manager::instance().register_command({"profile"}, + "profile - performance profiling", textp.str().c_str(), profiler_output_handler); - command_manager::instance().register_command({"pd", "PD", "profiledata", "ProfileData"}, + command_manager::instance().register_command({"profiledata"}, "profiler data - get appointed data, using by pjs", textpd.str().c_str(), profiler_data_handler); command_manager::instance().register_command( - {"profiler.query", "pq"}, - "profiler.query|pq - query profiling data, output in json format", + {"profiler.query"}, + "profiler.query - query profiling data, output in json format", textquery.str().c_str(), query_data_handler); } diff --git a/src/utils/command_manager.cpp b/src/utils/command_manager.cpp index b2b77f4d47..3f2160074f 100644 --- a/src/utils/command_manager.cpp +++ b/src/utils/command_manager.cpp @@ -25,11 +25,12 @@ */ #include -#include #include +#include #include #include +#include namespace dsn { @@ -38,29 +39,20 @@ dsn_handle_t command_manager::register_command(const std::vector &c const std::string &help_long, command_handler handler) { - utils::auto_write_lock l(_lock); - bool is_valid_cmd = false; + // TODO(wutao1): refactor the arg `commands` to single string rather than string-list. + dcheck_eq(commands.size(), 1); + const std::string &cmd = commands[0]; + dassert(!cmd.empty(), "should not register empty command"); - for (const std::string &cmd : commands) { - if (!cmd.empty()) { - is_valid_cmd = true; - auto it = _handlers.find(cmd); - dassert(it == _handlers.end(), "command '%s' already regisered", cmd.c_str()); - } - } - dassert(is_valid_cmd, "should not register empty command"); + utils::auto_write_lock l(_lock); - command_instance *c = new command_instance(); + auto c = new command_instance(); c->commands = commands; c->help_long = help_long; c->help_short = help_one_line; - c->handler = handler; + c->handler = std::move(handler); - for (const std::string &cmd : commands) { - if (!cmd.empty()) { - _handlers[cmd] = c; - } - } + _handlers[cmd] = c; return c; } @@ -82,31 +74,31 @@ bool command_manager::run_command(const std::string &cmd, { utils::auto_read_lock l(_lock); auto it = _handlers.find(cmd); - if (it != _handlers.end()) + if (it != _handlers.end()) { h = it->second; + } } if (h == nullptr) { output = std::string("unknown command '") + cmd + "'"; return false; - } else { - output = h->handler(args); - return true; } + output = h->handler(args); + return true; } command_manager::command_manager() { - register_command({"help", "h", "H", "Help"}, - "help|Help|h|H [command] - display help information", + register_command({"help"}, + "help - display help information", "", [this](const std::vector &args) { std::stringstream ss; if (args.size() == 0) { utils::auto_read_lock l(_lock); - for (const auto &c : this->_handlers) { - ss << c.second->help_short << std::endl; + for (const auto &c : _handlers) { + ss << c.first << ": " << c.second->help_short << std::endl; } } else { utils::auto_read_lock l(_lock); @@ -123,52 +115,6 @@ command_manager::command_manager() return ss.str(); }); - - register_command( - {"repeat", "r", "R", "Repeat"}, - "repeat|Repeat|r|R interval_seconds max_count command - execute command periodically", - "repeat|Repeat|r|R interval_seconds max_count command - execute command every interval " - "seconds, to the max count as max_count (0 for infinite)", - [this](const std::vector &args) { - std::stringstream ss; - - if (args.size() < 3) { - return "insufficient arguments"; - } - - int interval_seconds = atoi(args[0].c_str()); - if (interval_seconds <= 0) { - return "invalid interval argument"; - } - - int max_count = atoi(args[1].c_str()); - if (max_count < 0) { - return "invalid max count"; - } - - if (max_count == 0) { - max_count = std::numeric_limits::max(); - } - - std::string cmd = args[2]; - std::vector largs; - for (int i = 3; i < (int)args.size(); i++) { - largs.push_back(args[i]); - } - - for (int i = 0; i < max_count; i++) { - std::string output; - auto r = this->run_command(cmd, largs, output); - - if (!r) { - break; - } - - std::this_thread::sleep_for(std::chrono::seconds(interval_seconds)); - } - - return "repeat command completed"; - }); } command_manager::~command_manager() { _handlers.clear(); } diff --git a/src/utils/test/command_manager.cpp b/src/utils/test/command_manager_test.cpp similarity index 67% rename from src/utils/test/command_manager.cpp rename to src/utils/test/command_manager_test.cpp index b1074849e0..45696b0225 100644 --- a/src/utils/test/command_manager.cpp +++ b/src/utils/test/command_manager_test.cpp @@ -24,40 +24,39 @@ * THE SOFTWARE. */ -/* - * Description: - * Unit-test for command_manager. - * - * Revision history: - * Nov., 2015, @qinzuoyan (Zuoyan Qin), first version - * xxxx-xx-xx, author, fix bug about xxx - */ - #include #include -using namespace ::dsn; +namespace dsn { -void command_manager_module_init() +struct command_manager_test : public ::testing::Test { - dsn::command_manager::instance().register_command( - {"test-cmd"}, - "test-cmd - just for command_manager unit-test", - "test-cmd arg1 arg2 ...", - [](const std::vector &args) { - std::stringstream ss; - ss << "test-cmd response: ["; - for (size_t i = 0; i < args.size(); ++i) { - if (i != 0) - ss << " "; - ss << args[i]; - } - ss << "]"; - return ss.str(); - }); -} + command_manager_test() + { + _test_cmd = command_manager::instance().register_command( + {"test-cmd"}, + "just for command_manager unit-test", + "test-cmd arg1 arg2 ...", + [](const std::vector &args) { + std::stringstream ss; + ss << "test-cmd response: ["; + for (size_t i = 0; i < args.size(); ++i) { + if (i != 0) + ss << " "; + ss << args[i]; + } + ss << "]"; + return ss.str(); + }); + } + + ~command_manager_test() override { command_manager::instance().deregister_command(_test_cmd); } -TEST(command_manager, exist_command) +private: + dsn_handle_t _test_cmd; +}; + +TEST_F(command_manager_test, exist_command) { const std::string cmd = "test-cmd"; const std::vector cmd_args{"this", "is", "test", "argument"}; @@ -68,7 +67,7 @@ TEST(command_manager, exist_command) ASSERT_EQ(output, expect_output); } -TEST(command_manager, not_exist_command) +TEST_F(command_manager_test, not_exist_command) { const std::string cmd = "not-exist-cmd"; const std::vector cmd_args{"arg1", "arg2"}; @@ -78,3 +77,5 @@ TEST(command_manager, not_exist_command) std::string expect_output = std::string("unknown command '") + cmd + "'"; ASSERT_EQ(output, expect_output); } + +} // namespace dsn diff --git a/src/utils/test/main.cpp b/src/utils/test/main.cpp index 075b224a1c..b50a9a1a0e 100644 --- a/src/utils/test/main.cpp +++ b/src/utils/test/main.cpp @@ -9,7 +9,6 @@ GTEST_API_ int main(int argc, char **argv) { testing::InitGoogleTest(&argc, argv); - command_manager_module_init(); // init logging dsn_log_init("dsn::tools::simple_logger", "./", nullptr); From 7d73816723aadf2ccd4679e8c88b9c35b9c902d1 Mon Sep 17 00:00:00 2001 From: neverchanje Date: Fri, 2 Apr 2021 19:16:03 +0800 Subject: [PATCH 2/3] fix: revert unnecessary modifications --- src/utils/command_manager.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/utils/command_manager.cpp b/src/utils/command_manager.cpp index 3f2160074f..4bc563c58b 100644 --- a/src/utils/command_manager.cpp +++ b/src/utils/command_manager.cpp @@ -74,17 +74,17 @@ bool command_manager::run_command(const std::string &cmd, { utils::auto_read_lock l(_lock); auto it = _handlers.find(cmd); - if (it != _handlers.end()) { + if (it != _handlers.end()) h = it->second; - } } if (h == nullptr) { output = std::string("unknown command '") + cmd + "'"; return false; + } else { + output = h->handler(args); + return true; } - output = h->handler(args); - return true; } command_manager::command_manager() @@ -98,7 +98,7 @@ command_manager::command_manager() if (args.size() == 0) { utils::auto_read_lock l(_lock); for (const auto &c : _handlers) { - ss << c.first << ": " << c.second->help_short << std::endl; + ss << c.second->help_short << std::endl; } } else { utils::auto_read_lock l(_lock); From f0585b57bc68cd682ad4072e98a13172b19c201a Mon Sep 17 00:00:00 2001 From: neverchanje Date: Tue, 20 Apr 2021 12:12:41 +0800 Subject: [PATCH 3/3] remove redundant comment --- src/utils/command_manager.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/utils/command_manager.cpp b/src/utils/command_manager.cpp index 4bc563c58b..ad448a807f 100644 --- a/src/utils/command_manager.cpp +++ b/src/utils/command_manager.cpp @@ -39,7 +39,6 @@ dsn_handle_t command_manager::register_command(const std::vector &c const std::string &help_long, command_handler handler) { - // TODO(wutao1): refactor the arg `commands` to single string rather than string-list. dcheck_eq(commands.size(), 1); const std::string &cmd = commands[0]; dassert(!cmd.empty(), "should not register empty command");