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 4: (14 comments) http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG@13 PS3, Line 13: : This means we can ship a single binary in the kudu : docker image or potentially the kudu-binary jar : reducing the size by appoximately 66%. > Does this also inflate the size of the `kudu` binary? If so, by how much? There is no size or dependency change in the kudu binary because it already depended on the master and tserver. http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG@22 PS3, Line 22: ng > by Done http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG@25 PS3, Line 25: ossible. > How does this work wrt the log_dir flag? Does it just allow users to specif This is an existing flag. It defines the name of the file/s created in the log dir. I will rework this patch to make this no longer a concern. See other comments. http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG@29 PS3, Line 29: ensure f > related Done http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/master/master_main.cc File src/kudu/master/master_main.cc: http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/master/master_main.cc@38 PS3, Line 38: : namespace kudu { > Can be dropped? Done http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: PS3: > Would strongly prefer not to duplicate master_main.cc here. Could we make M I will rework this to ensure the code only lives in one place. http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc@257 PS3, Line 257: "include in output.\nPossible values: uuid, " > Should be probably explain here (or in ExtraDescription) that: Done http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc@263 PS3, Line 263: > I agree (the rest of the CLI treats these that way) but if you're going to Done http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc@268 PS3, Line 268: .AddAction(std::move(set_flag)) : .AddAction(std::move(start)) > Impala consolidated all their binaries; it's worth checking out how they di I may be able to do something clever by deferring the call to kudu::InitGoogleLoggingSafe(prog_name); in tool_main.cc so that I have an opportunity to set the log_filename before it is set in logging.cc. I like the idea of not only shipping a single binary, but not having multiple files just for the sake of argv[0] if I can help it. http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc@270 PS3, Line 270: .AddAction(std::move(status)) > BTW, why don't we just change the default value to 'kudu-master' for 'kudu See my comment above. The initialization of logging was happening before I had a chance to set the correct default. Originally I wasn't sure it was worth the effort to change, but now I think I will make this patch handle the log_filename. http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_tserver.cc File src/kudu/tools/tool_action_tserver.cc: http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_tserver.cc@251 PS3, Line 251: unique_ptr<Action> list_tservers = > Indentation. Done http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_tserver.cc@261 PS3, Line 261: .AddOptionalParameter("timeout_ms") : .Build(); > I'd drop these; keeping them in makes this read more and more like a list o I decided to put this here because it matches our documentation here: https://kudu.apache.org/docs/configuration.html#_configuring_tablet_servers http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/util/flags.h File src/kudu/util/flags.h: http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/util/flags.h@75 PS3, Line 75: GFlagsMap GetFlagsMap(); : : enum class TriStateFlag { > Probably easiest to document one of these two variants as "Like the above [ I found I could remove the old version. http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/util/flags.cc File src/kudu/util/flags.cc: http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/util/flags.cc@578 PS3, Line 578: flags_by_name.emplace(flag.name, std::move(flag)); > Should be possible to share the implementation with the existing variant. I found with all the changes I could remove the old version. -- 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: 4 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 05:25:06 +0000 Gerrit-HasComments: Yes
