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 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG@29 PS3, Line 29: releated related 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: DECLARE_int32(webserver_port); : DECLARE_string(rpc_bind_addresses); Can be dropped? 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 MasterMain from master_main.cc non-static and call it from the tool? I see there are some differences (i.e. whether to parse gflags and initialize logging) but surely we could parameterize that? Same for tserver. http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc@257 PS3, Line 257: .Description("Start a Kudu Master") Should be probably explain here (or in ExtraDescription) that: 1. The CLI tool will run until interrupted. 2. The Master will run on this machine (i.e. not somewhere remote). Same for the tserver variant. http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc@263 PS3, Line 263: // Even though fs_wal_dir is required, we don't want it to be positional argument. I agree (the rest of the CLI treats these that way) but if you're going to add this comment, it'd be good to also explain why. http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc@268 PS3, Line 268: // In logging.cc the `log_filename` flag is set to the binary name (`kudu`) : // by default. It may be common for users to override this. Impala consolidated all their binaries; it's worth checking out how they did that. I believe they now use symlinks (i.e. 'impalad' is the real binary and 'catalogd' -> 'impalad'). This affects argv[0] and could be a nice way to switch log_filename based on the binary being used: $ cat test.c #include <stdio.h> int main(int argc, char* argv[]) { printf("%s\n", argv[0]); return 0; } $ gcc -o test test.c $ ln -s test foo $ ./test ./test $ ./foo ./foo In the basic CLI case it's not going to help (i.e. when all you have is a 'kudu' binary). But in any packaged scenario where you can maintain symlinks (i.e. system packages, Docker images, kudu-binary JAR), this could be a simpler solution rather than forcing users to specify 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: .Description("Start a Kudu Tablet Server") Indentation. http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_tserver.cc@261 PS3, Line 261: .AddOptionalParameter("block_cache_capacity_mb") : .AddOptionalParameter("memory_limit_hard_bytes") I'd drop these; keeping them in makes this read more and more like a list of all stable params, and it'll be annoying to remember to update this list when stabilizing other params elsewhere. 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: // Get all the flags different their defaults. The output is a nicely : // formatted string with --flag=value pairs per line. Redact any flags that : // are tagged as sensitive, if redaction is enabled. Probably easiest to document one of these two variants as "Like the above [or like the below or whatever], but...". 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: string GetNonDefaultFlags() { Should be possible to share the implementation with the existing variant. -- 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: 3 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 19 Feb 2019 22:57:50 +0000 Gerrit-HasComments: Yes
