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 11: (5 comments) http://gerrit.cloudera.org:8080/#/c/12517/11/src/kudu/tools/tool_action.h File src/kudu/tools/tool_action.h: http://gerrit.cloudera.org:8080/#/c/12517/11/src/kudu/tools/tool_action.h@212 PS11, Line 212: ActionBuilder& ProgramName(const std::string& program_name); > what do you think about abusing gflags here and using SetArgv and ProgramIn We can't call SetArgv and ProgramInvocationName here at build time, it needs to be called at runtime if and only if this is the action being run. We could call it in Action::Run where I call InitGoogleLoggingSafe, though I sort of like maintaining Argv0 as kudu so we can know which binary is actually called. http://gerrit.cloudera.org:8080/#/c/12517/9/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: http://gerrit.cloudera.org:8080/#/c/12517/9/src/kudu/tools/tool_action_master.cc@83 PS9, Line 83: "all", google::FlagSettingMode::SET_FLAGS_DEFAULT)); > Nit: wrapping Done http://gerrit.cloudera.org:8080/#/c/12517/9/src/kudu/tools/tool_action_tserver.cc File src/kudu/tools/tool_action_tserver.cc: http://gerrit.cloudera.org:8080/#/c/12517/9/src/kudu/tools/tool_action_tserver.cc@82 PS9, Line 82: "all", google::FlagSettingMode::SET_FLAGS_DEFAULT)); > Nit: wrapping Done http://gerrit.cloudera.org:8080/#/c/12517/11/src/kudu/tools/tool_action_tserver.cc File src/kudu/tools/tool_action_tserver.cc: http://gerrit.cloudera.org:8080/#/c/12517/11/src/kudu/tools/tool_action_tserver.cc@216 PS11, Line 216: AddOptionalParameter > do you think we could do this by tagging flags? eg TAG_FLAG(fs_metadata_dir Yeah, that could be useful and should definitely be possible. I copied the flags defined here: https://kudu.apache.org/docs/configuration.html#_configuring_tablet_servers I would prefer to consider that in a later patch though given this patch has gotten pretty large. http://gerrit.cloudera.org:8080/#/c/12517/11/src/kudu/util/minidump.cc File src/kudu/util/minidump.cc: http://gerrit.cloudera.org:8080/#/c/12517/11/src/kudu/util/minidump.cc@217 PS11, Line 217: const char* program_name = !FLAGS_log_filename.empty() ? FLAGS_log_filename.c_str() : : gflags::ProgramInvocationShortName(); > Isn't log_filename initialized to gflags::ProgramInvocationShortName() in l It is when used in the real world when logging is initialized, but it isn't in the unit tests. -- 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: 11 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 22:53:41 +0000 Gerrit-HasComments: Yes
