Grant Henke 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: (14 comments) Still working on logging initialization issues, but addressed the review 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 Done 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. Done 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. Done 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. " > I think 'run' would be fine. Changed to run. Using & and also nohup works as expected. 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 namespac Done 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::... Done 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? That is what I am working on handling now. Currently it mostly is. 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 Done 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 Done 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. Done 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. Done 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 Done http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/util/rolling_log.h File src/kudu/util/rolling_log.h: http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/util/rolling_log.h@58 PS7, Line 58: RollingLog(Env* env, std::string log_dir, std::string program_name_, std::string log_name); > warning: function 'kudu::RollingLog::RollingLog' has a definition with diff Done http://gerrit.cloudera.org:8080/#/c/12517/7/src/kudu/util/rolling_log.h@58 PS7, Line 58: RollingLog(Env* env, std::string log_dir, std::string program_name_, std::string log_name); > warning: invalid case style for parameter 'program_name_' [readability-iden Done -- 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 19:48:21 +0000 Gerrit-HasComments: Yes
