Bankim Bhavsar 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) 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 f Done 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.? Oh yes! Good point. Done 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? I used structured binding but removed it because of the unused variable warning. See earlier patchsets. http://gerrit.cloudera.org:8080/#/c/17267/3/src/kudu/tools/tool_action_master.cc@392 PS3, Line 392: push_back > nit: emplace_back? Done 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 cl Done 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 Done 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 Configuratio Done -- 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: Tue, 20 Apr 2021 05:12:13 +0000 Gerrit-HasComments: Yes
