Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18979 )
Change subject: Enable kudu cli to accept specific leader to step down Takes three arguments to identify the leader: 1. Leader uuid 2. Leader Hostname 3. Leader Host port This can be useful in scenario when master is out of sync and user wants to choose a specific leader ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/18979/1//COMMIT_MSG Commit Message: PS1: Please add a test for the newly added functionality. For the references, take a look at existing test scenarios in kudu-admin-test.cc http://gerrit.cloudera.org:8080/#/c/18979/1//COMMIT_MSG@6 PS1, Line 6: Please update the description to conform with the guidelines at https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines (you could find the reference to that doc at https://kudu.apache.org/docs/contributing.html) http://gerrit.cloudera.org:8080/#/c/18979/1/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: http://gerrit.cloudera.org:8080/#/c/18979/1/src/kudu/tools/tool_action_tablet.cc@63 PS1, Line 63: nit for here and below: why to add this trailing space? http://gerrit.cloudera.org:8080/#/c/18979/1/src/kudu/tools/tool_action_tablet.cc@66 PS1, Line 66: DEFINE_string(current_leader_hp_port, "", : "Port of the current leader. "); Why string, not integer for port? Also, please add a validator for the values specified -- check the DEFINE_validator() macro (could you existing code to see examples of using it). http://gerrit.cloudera.org:8080/#/c/18979/1/src/kudu/tools/tool_action_tablet.cc@165 PS1, Line 165: nit for here and below: wrong indent, should be four spaces from the beginning of the prior line (or six spaces from the left margin), see examples in the code style guide at https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions and other related sections. http://gerrit.cloudera.org:8080/#/c/18979/1/src/kudu/tools/tool_action_tablet.cc@167 PS1, Line 167: const optional<string> current_leader_hp_host = : FLAGS_current_leader_hp_host.empty() ? nullopt : : make_optional(FLAGS_current_leader_hp_host); : const optional<string> current_leader_hp_port = : FLAGS_current_leader_hp_port.empty() ? nullopt : : make_optional(FLAGS_current_leader_hp_port); If tablet leader's RPC endpoint isn't specified, is it possible to deduce it from the information that ksck has on hand given leader UUID? http://gerrit.cloudera.org:8080/#/c/18979/1/src/kudu/tools/tool_action_tablet.cc@174 PS1, Line 174: nit: extra space in extra line http://gerrit.cloudera.org:8080/#/c/18979/1/src/kudu/tools/tool_action_tablet.cc@175 PS1, Line 175: current_leader_hp_host && current_leader_hp_port What if only one is specified? If only port specified, that should be an error, I guess. You could use GROUP_FLAG_VALIDATOR to check validity of a group of flags specified. If only the host specified, the port may default to tablet server's RPC port in such case. -- To view, visit http://gerrit.cloudera.org:8080/18979 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5f9631ec2001a7c030846cb835d540254b840e5c Gerrit-Change-Number: 18979 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 13 Sep 2022 16:21:26 +0000 Gerrit-HasComments: Yes
