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 7: (13 comments) http://gerrit.cloudera.org:8080/#/c/19024/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19024/7//COMMIT_MSG@7 PS7, Line 7: cli nit: CLI http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/kudu-admin-test.cc@1362 PS7, Line 1362: to new flag nit: this becomes non-relevant if the text is read outside of this patch (e.g., if anybody is reading the code once it's submitted into the repo), so drop the 'new' part http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/kudu-admin-test.cc@1362 PS7, Line 1362: argument an argument http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/kudu-admin-test.cc@1370 PS7, Line 1370: const vector<string> kTserverFlags = { : "--enable_leader_failure_detection=false", : }; Would be great to have a comment to explain why to add this option. http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/kudu-admin-test.cc@1394 PS7, Line 1394: // Ask the leader to transfer leadership to a specific peer. Why to have this step if the ultimate goal is to check how --current_leader_uuid works? Why not to rely on the GetLeaderReplicaWithRetries() to find the current leader replica and then run 'kudu tablet leader_step_down ... --current_leader_uuid=<observer_leader_uuid>'? Would be great to add a comment about that. http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/kudu-admin-test.cc@1404 PS7, Line 1404: ASSERT_TRUE(s.ok()) << s.ToString(); nit for here and below: use ASSERT_OK(s) instead http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/kudu-admin-test.cc@1422 PS7, Line 1422: leader a leader http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/kudu-admin-test.cc@1422 PS7, Line 1422: some other node a replica at other node http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/kudu-admin-test.cc@1429 PS7, Line 1429: } Do you mind adding a "negative" scenario running 'kudu tablet leader_step_down' with --current_leader_uuid is set to UUID of tablet server where a follower, not leader replica is currently running? http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/tool_action_tablet.cc@174 PS7, Line 174: if (new_leader_uuid) Why to differentiate between whether new_leader_uuid is specified or not if the problem is pertained only to the fact that current leader isn't found? http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/tool_action_tablet.cc@176 PS7, Line 176: style nit: there should be 4 spaces here, not 6; see https://google.github.io/styleguide/cppguide.html#Function_Calls for details http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/tool_replica_util.h File src/kudu/tools/tool_replica_util.h: http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/tool_replica_util.h@95 PS7, Line 95: Get information on the given replica uuid for the specified tablet How about: Get host/port information of the tablet server with the specified UUID 'uuid' that hosts a replica of the tablet identified by 'tablet_id' ? http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/tool_replica_util.cc File src/kudu/tools/tool_replica_util.cc: http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/tool_replica_util.cc@209 PS7, Line 209: Substitute("No replica host found for tablet $0", : tablet_id) How about: Substitute("no replica of tablet $0 is hosted by tablet server $1", tablet_id, uuid) ? -- 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: 7 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[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: Mon, 05 Dec 2022 22:12:26 +0000 Gerrit-HasComments: Yes
