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 9: (15 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 Done 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: an argum > an argument Done http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/kudu-admin-test.cc@1362 PS7, Line 1362: nt to flag > nit: this becomes non-relevant if the text is read outside of this patch (e Done http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/kudu-admin-test.cc@1370 PS7, Line 1370: : // For deterministic control over manual leader selection : co > Would be great to have a comment to explain why to add this option. Done http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/kudu-admin-test.cc@1394 PS7, Line 1394: ASSERT_EQ(leader->uuid(), master_observed_leader->uuid()); > Why to have this step if the ultimate goal is to check how --current_leader Done http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/kudu-admin-test.cc@1404 PS7, Line 1404: // where master configuration is out > nit for here and below: use ASSERT_OK(s) instead Done http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/kudu-admin-test.cc@1422 PS7, Line 1422: > a leader Done http://gerrit.cloudera.org:8080/#/c/19024/7/src/kudu/tools/kudu-admin-test.cc@1422 PS7, Line 1422: > a replica at other node Done 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: string stderr; : > Don't we have a leader deterministically chosen already by calling StartEle I guess that is just the convention tests follow in this suite. I have removed this intermediate step of electing a leader and resumed next step by simply assigning new_leader_uuid the uuid of leader i.e. leader->uuid(). That also works fine. http://gerrit.cloudera.org:8080/#/c/19024/8/src/kudu/tools/kudu-admin-test.cc@1449 PS8, Line 1449: FLAGS_num_replicas = > I'd think we want to check for some specific error status here, right? May Done http://gerrit.cloudera.org:8080/#/c/19024/8/src/kudu/tools/kudu-admin-test.cc@1451 PS8, Line 1451: : // Wait for the tablet to be running. : vector<TServerDetails*> tservers; : AppendValuesFromMap(tablet_servers_, &tservers); : ASS > Why do we need this ASSERT_EVENTUALLY scope? I'd think that no change in t This was added just to ensure that leader remains the same. I can drop this as we are already checking for error above at line 1449. 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: *curr > Why to differentiate between whether new_leader_uuid is specified or not if Done 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. Done 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 host/port information of the tablet server with the specified > How about: Done 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 of tablet $0 is hosted by " : "tablet se > How about: Done -- 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: 9 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 09:49:47 +0000 Gerrit-HasComments: Yes
