Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18877 )
Change subject: [client] Fix a kudu cpp client bug when using replica_selection policy ...................................................................... Patch Set 12: (15 comments) http://gerrit.cloudera.org:8080/#/c/18877/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18877/12//COMMIT_MSG@9 PS12, Line 9: cpp C++ http://gerrit.cloudera.org:8080/#/c/18877/12//COMMIT_MSG@15 PS12, Line 15: unit tests Is there a test which would fail for the scenario described above? http://gerrit.cloudera.org:8080/#/c/18877/12//COMMIT_MSG@15 PS12, Line 15: fix fixes http://gerrit.cloudera.org:8080/#/c/18877/12//COMMIT_MSG@15 PS12, Line 15: add adds http://gerrit.cloudera.org:8080/#/c/18877/12//COMMIT_MSG@15 PS12, Line 15: at client lib in the client library http://gerrit.cloudera.org:8080/#/c/18877/8/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: http://gerrit.cloudera.org:8080/#/c/18877/8/src/kudu/client/scan_token-test.cc@210 PS8, Line 210: one and empty li What is 'refactor'? Consider some meaningful name instead. http://gerrit.cloudera.org:8080/#/c/18877/12/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: http://gerrit.cloudera.org:8080/#/c/18877/12/src/kudu/client/scan_token-test.cc@1495 PS12, Line 1495: TEST_F(ScanTokenTest, TestReplicaSelection) { Could you add a concise description what this test is checking for and how it's doing that (just the essence)? http://gerrit.cloudera.org:8080/#/c/18877/12/src/kudu/client/scan_token-test.cc@1502 PS12, Line 1502: kReplicaFactor nit: what's 'replica factor'? Maybe, you meant 'replication factor'? If so, maybe name this 'kReplicationFactor' to follow the convention used in other tests. 'kReplicasNum' or 'kNumReplicas' might be good options as well. http://gerrit.cloudera.org:8080/#/c/18877/12/src/kudu/client/scan_token-test.cc@1506 PS12, Line 1506: for scan 'for scanning' or 'to scan' http://gerrit.cloudera.org:8080/#/c/18877/12/src/kudu/client/scan_token-test.cc@1516 PS12, Line 1516: ASSERT_EVENTUALLY_WITH_TIMEOUT([&]() { ASSERT_GE(workload.rows_inserted(), kCount); }, 300); nit for here and below: this line is too long -- please strive to have at most 80 characters in one line for C++ code; please check out https://google.github.io/styleguide/cppguide.html#Line_Length http://gerrit.cloudera.org:8080/#/c/18877/12/src/kudu/client/scan_token-test.cc@1533 PS12, Line 1533: count Count of what? http://gerrit.cloudera.org:8080/#/c/18877/12/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/18877/12/src/kudu/common/common.proto@316 PS12, Line 316: at cpp client in the Kudu C++ client http://gerrit.cloudera.org:8080/#/c/18877/12/src/kudu/common/common.proto@316 PS12, Line 316: at some certain situations in particular situations http://gerrit.cloudera.org:8080/#/c/18877/12/src/kudu/tools/table_scanner.cc File src/kudu/tools/table_scanner.cc: http://gerrit.cloudera.org:8080/#/c/18877/12/src/kudu/tools/table_scanner.cc@172 PS12, Line 172: constexpr const char* const kReplicaSelectionClosest = "closest"; : constexpr const char* const kReplicaSelectionLeader = "leader"; : constexpr const char* const kReplicaSelectionFirst = "first"; nit: order these lines by the name of the constant (alphabetically, ASCII) http://gerrit.cloudera.org:8080/#/c/18877/12/src/kudu/util/test_util.h File src/kudu/util/test_util.h: http://gerrit.cloudera.org:8080/#/c/18877/12/src/kudu/util/test_util.h@47 PS12, Line 47: timeout Why not to provide timeout as MonoDelta as is in this second macro parameter? Otherwise, there might be some confusion in what units the 'timeout' parameter is assumed to be. -- 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: 12 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: Wed, 14 Sep 2022 21:50:03 +0000 Gerrit-HasComments: Yes
