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 8: (3 comments) Almost there! Just a few questions left to address. http://gerrit.cloudera.org:8080/#/c/19024/8/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/19024/8/src/kudu/tools/kudu-admin-test.cc@1397 PS8, Line 1397: // This is to choose a specific peer that is expected to be new leader : // so that we can use the chosen new leader for next test deterministically. Don't we have a leader deterministically chosen already by calling StartElection() at line 1389 above? Seeing this makes me think there is something else behind running the CLI tool with the --new_leader_uuid option set. If so, it would be great to add a comment about, otherwise why to run this at all? http://gerrit.cloudera.org:8080/#/c/19024/8/src/kudu/tools/kudu-admin-test.cc@1449 PS8, Line 1449: ASSERT_FALSE(s.ok()) I'd think we want to check for some specific error status here, right? Maybe, replace this with ASSERT_TRUE(s.IsRuntimError()) << s.ToString()? http://gerrit.cloudera.org:8080/#/c/19024/8/src/kudu/tools/kudu-admin-test.cc@1451 PS8, Line 1451: // Eventually, leader should remain the same. : ASSERT_EVENTUALLY([&]() { : ASSERT_OK(GetLeaderReplicaWithRetries(tablet_id_, &master_observed_leader)); : ASSERT_TRUE(ContainsKey(possible_new_leaders, master_observed_leader->uuid())); : }); Why do we need this ASSERT_EVENTUALLY scope? I'd think that no change in the replica leadership happens, and the leader is the same as stored in the 'master_observed_leader' variable at line 1436 above. If there is a particular reason behind wrapping this into ASSERT_EVENTUALLY, please add a comment about that, otherwise drop ASSERT_EVENTUALLY, maybe? -- 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: 8 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: Thu, 08 Dec 2022 02:06:39 +0000 Gerrit-HasComments: Yes
