You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2022/12/05 22:12:26 UTC
[kudu-CR] KUDU-3403 Enable kudu cli to accept specific leader to step down
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 <ar...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 05 Dec 2022 22:12:26 +0000
Gerrit-HasComments: Yes