Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/19024 )
Change subject: KUDU-3403 Enable kudu cli to accept specific leader to step down ...................................................................... Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/19024/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19024/5//COMMIT_MSG@10 PS5, Line 10: deduct > deduce? Done http://gerrit.cloudera.org:8080/#/c/19024/5//COMMIT_MSG@13 PS5, Line 13: leader in the master config. > nit: ... leader in the tablet configuration known to master. Done http://gerrit.cloudera.org:8080/#/c/19024/5/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: http://gerrit.cloudera.org:8080/#/c/19024/5/src/kudu/tools/tool_action_tablet.cc@160 PS5, Line 160: > nit: I guess for line continuations there should be 4 space indent, similar There is no clear guidance on line continuation in style guide (for regular statements case) so I went with what new_leader_uuid(line 153) had, i.e. 2 spaces. I am fine with 4 spaces. By that logic, ": make_optional(FLAGS_current_leader_uuid);" should also have 4 space indent ? http://gerrit.cloudera.org:8080/#/c/19024/5/src/kudu/tools/tool_replica_util.h File src/kudu/tools/tool_replica_util.h: http://gerrit.cloudera.org:8080/#/c/19024/5/src/kudu/tools/tool_replica_util.h@98 PS5, Line 98: for given leader_uuid > What if 'leader_uuid' is set to 'nullopt'? Caller makes call to this function only when leader_uuid value is present. I guess it would make more sense to make it string type rather than optional. http://gerrit.cloudera.org:8080/#/c/19024/5/src/kudu/tools/tool_replica_util.cc File src/kudu/tools/tool_replica_util.cc: http://gerrit.cloudera.org:8080/#/c/19024/5/src/kudu/tools/tool_replica_util.cc@194 PS5, Line 194: const string& tablet_id, : const std::optional<std::string>& leader_uuid, : HostPort* leader_hp) { > nit: the indent is off for these 3 lines Done http://gerrit.cloudera.org:8080/#/c/19024/5/src/kudu/tools/tool_replica_util.cc@202 PS5, Line 202: if (leader_uuid && *leader_uuid == r->ts().uuid()) > This logic looks a bit strange to me (so called 'bad smell'): with leader_u We come here only when there leader_uuid has some value. Since leader_uuid is input parameter, it make sense to just pass as a string and compare the value instead of passing as optional, to avoid confusion. Something like this: Status GetTabletLeaderHostInfo(const client::sp::shared_ptr<KuduClient>& client, const string& tablet_id, const std::string& leader_uuid, HostPort* leader_hp) { KuduTablet* tablet_raw = nullptr; RETURN_NOT_OK(client->GetTablet(tablet_id, &tablet_raw)); unique_ptr<KuduTablet> tablet(tablet_raw); for (const auto* r : tablet->replicas()) { if (leader_uuid == r->ts().uuid()) { Even with this logic, there is a possibility that user enter a uuid value that is not valid i.e. no replica exists with that uuid. In such case, we will just return Status::NotFound(). -- To view, visit http://gerrit.cloudera.org:8080/19024 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I40569faa40a8173c51504c7567aa84a6ae1fb64a Gerrit-Change-Number: 19024 Gerrit-PatchSet: 5 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Tue, 11 Oct 2022 13:59:42 +0000 Gerrit-HasComments: Yes
