Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18877 )
Change subject: [client] Fix a kudu c++ client bug when using replica_selection policy ...................................................................... Patch Set 18: (11 comments) http://gerrit.cloudera.org:8080/#/c/18877/18//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18877/18//COMMIT_MSG@10 PS18, Line 10: Eg command: How about: For example, 'kudu perf table_scan $master_list $table -replica_selection="LEADER"' specifies LEADER_ONLY policy, but the effective policy applied was CLOSEST_REPLICA. http://gerrit.cloudera.org:8080/#/c/18877/18//COMMIT_MSG@10 PS18, Line 10: is was http://gerrit.cloudera.org:8080/#/c/18877/18/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: http://gerrit.cloudera.org:8080/#/c/18877/18/src/kudu/client/scan_token-test.cc@152 PS18, Line 152: KuduClient::ReplicaSelection::LEADER_ONLY Is this backward-compatible with already existing tests? IIRC, the default mode for KuduScanner was CLOSEST_REPLICA. http://gerrit.cloudera.org:8080/#/c/18877/18/src/kudu/client/scan_token-test.cc@206 PS18, Line 206: replica_factor nit: replication_factor In Kudu project this has been called 'replication_factor' for a long time already, so let's keep that terminology :) http://gerrit.cloudera.org:8080/#/c/18877/18/src/kudu/client/scan_token-test.cc@1495 PS18, Line 1495: check checks http://gerrit.cloudera.org:8080/#/c/18877/18/src/kudu/client/scan_token-test.cc@1495 PS18, Line 1495: replica_selection strategy work well replica selection works as expected http://gerrit.cloudera.org:8080/#/c/18877/18/src/kudu/client/scan_token-test.cc@1495 PS18, Line 1495: The This http://gerrit.cloudera.org:8080/#/c/18877/18/src/kudu/client/scan_token-test.cc@1502 PS18, Line 1502: Get current scanner satistics info, then: Get current scanner's statistics, and then: http://gerrit.cloudera.org:8080/#/c/18877/18/src/kudu/client/scan_token-test.cc@1505 PS18, Line 1505: // 2. If replica selection is CLOSEST_REPLICA, after scanned, at least one tserver's : // increment number of scanners is not the numbers of leaders. This is a little risk when : // CLOSEST_REPLICA the same as LEADER_ONLY, the probability is very low because of random : // pick, at our test case has 10 partitions. There is still a chance this might happen, and I guess that might be the source of flakiness for this test scenario. Did you try to run this test scenario many times to make sure it's not flaky? Instead, maybe consider using location assignment to have a deterministic test scenario? You could take a look at ClientLocationAssignmentITest.Basic in location_assignment-itest.cc for reference. http://gerrit.cloudera.org:8080/#/c/18877/18/src/kudu/tools/table_scanner.cc File src/kudu/tools/table_scanner.cc: http://gerrit.cloudera.org:8080/#/c/18877/18/src/kudu/tools/table_scanner.cc@179 PS18, Line 179: kReplicaSelectionClosest, : kReplicaSelectionLeader, : kReplicaSelectionFirst, nit: the same as for the kReplicaSelectionXxx above, please order the elements of this enum alphabetically. http://gerrit.cloudera.org:8080/#/c/18877/18/src/kudu/util/test_util.h File src/kudu/util/test_util.h: http://gerrit.cloudera.org:8080/#/c/18877/18/src/kudu/util/test_util.h@47 PS18, Line 47: mono_delta_s nit: once this parameter is of MonoDelta type, the '_s' suffix isn't needed -- please drop it Wouldn't "timeout" be a better name for this parameter? -- To view, visit http://gerrit.cloudera.org:8080/18877 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I413f99b6a0b6082c5453358b8333913e4c6264c2 Gerrit-Change-Number: 18877 Gerrit-PatchSet: 18 Gerrit-Owner: Yuqi Du <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Mon, 26 Sep 2022 02:15:00 +0000 Gerrit-HasComments: Yes
