Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17267 )
Change subject: [master][tool] KUDU-2181 Tool to orchestrate adding a master ...................................................................... Patch Set 3: (7 comments) Overall looks pretty good. Mostly nits, and one test suggestion http://gerrit.cloudera.org:8080/#/c/17267/3/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: http://gerrit.cloudera.org:8080/#/c/17267/3/src/kudu/tools/tool_action_master.cc@374 PS3, Line 374: : // Get the flags that'll be needed to bring up new master and for system catalog copy. : GFlagsMap flags_map = GetFlagsMap(); Do we have a test that requires this? E.g. maybe adding/removing a master from a secure cluster? http://gerrit.cloudera.org:8080/#/c/17267/3/src/kudu/tools/tool_action_master.cc@382 PS3, Line 382: string fs_wal_dir; : RETURN_NOT_OK(GetFlagValue(flags_map, "fs_wal_dir", &fs_wal_dir)); : string fs_data_dirs; : RETURN_NOT_OK(GetFlagValue(flags_map, "fs_data_dirs", &fs_data_dirs)); Could we DECLARE_string() these and use FLAGS_fs_wal_dir, etc.? http://gerrit.cloudera.org:8080/#/c/17267/3/src/kudu/tools/tool_action_master.cc@390 PS3, Line 390: name_flag_pair nit: could use structured bindings for these? http://gerrit.cloudera.org:8080/#/c/17267/3/src/kudu/tools/tool_action_master.cc@392 PS3, Line 392: push_back nit: emplace_back? http://gerrit.cloudera.org:8080/#/c/17267/3/src/kudu/tools/tool_action_master.cc@815 PS3, Line 815: "This is an advanced command that orchestrates the workflow to bring up and add a " : "master to the Kudu cluster. It must be run locally on the master being added and " : "not on the leader master.\n\n" nit: can you also mention that this doesn't leave a living master in the cluster, and that callers should subsequently start a master using the same master flags as those used in this tool? http://gerrit.cloudera.org:8080/#/c/17267/3/src/kudu/tools/tool_action_master.cc@820 PS3, Line 820: The most common configuration flags used to bring up the new master are described " : "below. nit: readers have to read between the lines to figure out that they should be passing all the flags they want on a new master. Can we explicitly state that here? http://gerrit.cloudera.org:8080/#/c/17267/3/src/kudu/tools/tool_action_master.cc@822 PS3, Line 822: https://kudu.apache.org/docs/configuration_reference.html"; : "#kudu-master_supported nit: shying from URLs, maybe refer to this as "the Kudu Master Configuration Reference"? -- To view, visit http://gerrit.cloudera.org:8080/17267 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f99cf2b3f1738c4127c7e7288beab61daf42e7b Gerrit-Change-Number: 17267 Gerrit-PatchSet: 3 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 09 Apr 2021 20:43:15 +0000 Gerrit-HasComments: Yes
