Alexey Serbin 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 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/19024/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19024/6//COMMIT_MSG@9 PS6, Line 9: uuid nit: UUID http://gerrit.cloudera.org:8080/#/c/19024/6/src/kudu/tools/tool_action_common.h File src/kudu/tools/tool_action_common.h: http://gerrit.cloudera.org:8080/#/c/19024/6/src/kudu/tools/tool_action_common.h@83 PS6, Line 83: extern const char* const kCurrentLeaderUUIDArg; nit: add an empty string after this line to separate the series of declarations from the code below http://gerrit.cloudera.org:8080/#/c/19024/6/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: http://gerrit.cloudera.org:8080/#/c/19024/6/src/kudu/tools/tool_action_tablet.cc@63 PS6, Line 63: is currently the leader nit: ... currently hosts leader replica of the tablet http://gerrit.cloudera.org:8080/#/c/19024/6/src/kudu/tools/tool_action_tablet.cc@159 PS6, Line 159: const optional<string> current_leader_uuid = : FLAGS_current_leader_uuid.empty() ? nullopt : : make_optional(FLAGS_current_leader_uuid); nit: could this be formatted as something below const auto current_leader_uuid = FLAGS_current_leader_uuid.empty() ? nullopt : make_optional(FLAGS_current_leader_uuid); http://gerrit.cloudera.org:8080/#/c/19024/6/src/kudu/tools/tool_action_tablet.cc@166 PS6, Line 166: Status s; nit: either move this variable under the 'if/then' scope, or move that RETURN_NOT_OK(s) out of each of the scope after if/else below http://gerrit.cloudera.org:8080/#/c/19024/6/src/kudu/tools/tool_action_tablet.cc@179 PS6, Line 179: // Otherwise, a new election should happen soon, which will achieve : // something like what the client wanted, so we'll exit gracefully. : cout << s.ToString() << endl; : return Status::OK(); I don't think this is applicable for this situation (unlike the case when calling GetTabletLeader()). The UUID provided might be simply pointing to wrong tablet server, and no leader election might be in sight for a very long time for the tablet in question. Should we report an error here and exit with non-OK instead? http://gerrit.cloudera.org:8080/#/c/19024/6/src/kudu/tools/tool_replica_util.h File src/kudu/tools/tool_replica_util.h: http://gerrit.cloudera.org:8080/#/c/19024/6/src/kudu/tools/tool_replica_util.h@95 PS6, Line 95: Get information on the current leader replica This comment is a bit confusing: there is nothing related to the leadership role of the replica in the implementation, actually. http://gerrit.cloudera.org:8080/#/c/19024/6/src/kudu/tools/tool_replica_util.cc File src/kudu/tools/tool_replica_util.cc: http://gerrit.cloudera.org:8080/#/c/19024/6/src/kudu/tools/tool_replica_util.cc@193 PS6, Line 193: GetTabletLeaderHostInfo >From the implementation, I didn't see anything that might be related to the >leadership role of the replica in question. Maybe, rename this function into >GetTabletReplicaHostInfo() then? -- 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: 6 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, 15 Nov 2022 03:56:33 +0000 Gerrit-HasComments: Yes
