Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode ...................................................................... Patch Set 4: (16 comments) http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc@18 PS4, Line 18: #include <assert.h> Switch to using ASSERT_TRUE() from assert() and remove this header. http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc@1197 PS4, Line 1197: "set_all_masters_flag.*Change a gflag value", nit: it's better to keep this sorted alphabetically -- move this new string to be after 'set_flag' http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc@1374 PS4, Line 1374: "set_all_tservers_flag.*Change a gflag value", nit: keep it sorted http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc@8109 PS4, Line 8109: INSTANTIATE_TEST_CASE_P(, SetClusterFlagTest, ::testing::Bool()); It would be great to add a test scenario to make sure that the new tools return non-OK status when setting flag fail at least for one server. http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc@8156 PS4, Line 8156: assert(item["flag"].IsString()); here and below: don't use assert in tests, use ASSERT_TRUE() instead http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@152 PS4, Line 152: master_addresses I guess this list should be retrieved from the leader master instead of relying on the input. See ListMasters() in tool_action_master.cc for the reference. http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@154 PS4, Line 154: (!s.ok()) It would help to have more information on the error per each record (e.g., add s.ToString()). http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@155 PS4, Line 155: std::cout This should be std::cerr, or maybe use LOG() instead. http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@155 PS4, Line 155: "Address: " << addr << " config: {" : << flag << ", " << value << "} set failed!" << std::endl; For better readability, use Substitute() here instead. http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@790 PS4, Line 790: set_all_masters_flag This sub-command is already in the "master" context, so maybe rename this to 'set_flag_for_all' or something similar. http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@791 PS4, Line 791: on all Kudu Masters nit: for all Kudu Masters in the cluster http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc File src/kudu/tools/tool_action_tserver.cc: http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc@152 PS4, Line 152: LOG(WARNING) It would be great to see what was the error for each of these (e.g., adding s.ToString() to each message could shed some light on that). http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc@152 PS4, Line 152: "Address: " << addr << " config: {" : << flag << ", " << value << "} set failed!" For better readability, consider using Substitute() instead. http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc@157 PS4, Line 157: TServer nit (try to keep the same terms in the output): Tablet Server http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc@429 PS4, Line 429: set_all_tservers_flag That's already tserver's context (kudu tserver <sub-command>), so maybe rename this sub-command into 'set_flag_for_all' http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc@430 PS4, Line 430: on all Kudu Tablet Servers nit: for all Kudu Tablet Servers in the cluster -- To view, visit http://gerrit.cloudera.org:8080/18506 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f Gerrit-Change-Number: 18506 Gerrit-PatchSet: 4 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Wed, 11 May 2022 01:50:08 +0000 Gerrit-HasComments: Yes
