Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12517 )
Change subject: [tools] Support starting master and tablet server via the kudu binary ...................................................................... Patch Set 7: (12 comments) http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/master/CMakeLists.txt File src/kudu/master/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/master/CMakeLists.txt@41 PS7, Line 41: master_runner.cc Nit: should come before master_service.cc http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/master/master_runner.h File src/kudu/master/master_runner.h: http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/master/master_runner.h@17 PS7, Line 17: #ifndef KUDU_MASTER_MASTER_RUNNER_H : #define KUDU_MASTER_MASTER_RUNNER_H Use #pragma once for new files. http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/master/master_runner.cc File src/kudu/master/master_runner.cc: http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/master/master_runner.cc@72 PS7, Line 72: std::string nondefault_flags = GetNonDefaultFlags(); Add a using for this. http://gerrit.cloudera.org:8080/#/c/12517/4/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: http://gerrit.cloudera.org:8080/#/c/12517/4/src/kudu/tools/tool_action_master.cc@218 PS4, Line 218: "The most common configuration flags are described below. " > Should the action be "run" or "exec" to better indicate that this will cont I think 'run' would be fine. You should also test what happens when you append '&' to the shell command line (i.e. detach from the tool). I presume it works fine, but it's a common enough use case that it'd be good to double check. http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/tools/tool_action_master.cc@92 PS7, Line 92: kudu::master::SetMasterFlagDefaults(); : kudu::master::RunMasterServer(); You should be able to just do master::... since you're in the kudu namespace already. http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/tools/tool_action_tserver.cc File src/kudu/tools/tool_action_tserver.cc: http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/tools/tool_action_tserver.cc@92 PS7, Line 92: kudu::tserver::SetTabletServerFlagDefaults(); : kudu::tserver::RunTabletServer(); Should be able to do tserver::... http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/tools/tool_main.cc File src/kudu/tools/tool_main.cc: http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/tools/tool_main.cc@264 PS7, Line 264: bool show_help = ParseCommandLineFlags(prog_name); Is it safe to call this before InitGoogleLoggingSafe? http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/tserver/tablet_server_runner.h File src/kudu/tserver/tablet_server_runner.h: http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/tserver/tablet_server_runner.h@17 PS7, Line 17: #ifndef KUDU_TSERVER_TABLET_SERVER_RUNNER_H : #define KUDU_TSERVER_TABLET_SERVER_RUNNER_H #pragma once http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/tserver/tablet_server_runner.cc File src/kudu/tserver/tablet_server_runner.cc: http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/tserver/tablet_server_runner.cc@68 PS7, Line 68: std::string nondefault_flags = GetNonDefaultFlags(); using http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/util/flags-test.cc File src/kudu/util/flags-test.cc: http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/util/flags-test.cc@56 PS7, Line 56: // Memorize the default flags : GFlagsMap default_flags = GetFlagsMap(); Can drop this from the test now. http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/util/flags.h File src/kudu/util/flags.h: http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/util/flags.h@70 PS7, Line 70: // Get all the flags different their defaults. The output is a nicely Probably should revert the change to this line. http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/util/minidump.cc File src/kudu/util/minidump.cc: http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/util/minidump.cc@212 PS7, Line 212: Add the log_filename, the executable name by default, Nit: "Add log_filename (the executable's name by default) to the path where..." -- To view, visit http://gerrit.cloudera.org:8080/12517 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3717cbac930b3506a76f7a51388c64afbcbb480e Gerrit-Change-Number: 12517 Gerrit-PatchSet: 7 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 20 Feb 2019 18:48:25 +0000 Gerrit-HasComments: Yes
