Yingchun Lai 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 10: (5 comments) http://gerrit.cloudera.org:8080/#/c/18877/10/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: http://gerrit.cloudera.org:8080/#/c/18877/10/src/kudu/client/scan_token-internal.cc@243 PS10, Line 243: WARN_NOT_OK Why just warn but not return error? http://gerrit.cloudera.org:8080/#/c/18877/10/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: http://gerrit.cloudera.org:8080/#/c/18877/10/src/kudu/client/scan_token-test.cc@209 PS10, Line 209: std::tuple<int, int> storage_parameters = {1, 1}) { How about separete into 2 parameters explicitly? http://gerrit.cloudera.org:8080/#/c/18877/10/src/kudu/client/scan_token-test.cc@1515 PS10, Line 1515: constexpr const int32_t kCount = 100000; : // Populate the table with data. : { : shared_ptr<KuduSession> session = client_->NewSession(); : session->SetTimeoutMillis(10000); : ASSERT_OK(session->SetFlushMode(KuduSession::MANUAL_FLUSH)); : : for (int i = 1; i <= kCount; ++i) { : unique_ptr<KuduInsert> insert(table->NewInsert()); : ASSERT_OK(insert->mutable_row()->SetInt64("key", i)); : ASSERT_OK(session->Apply(insert.release())); : if (i % 128 == 0) { : ASSERT_OK(session->Flush()); : } : } : ASSERT_OK(session->Flush()); : } Is it OK to use TestWorkload to populate data? http://gerrit.cloudera.org:8080/#/c/18877/10/src/kudu/client/scan_token-test.cc@1542 PS10, Line 1542: // first_replica : permanent_uuid -> count : std::map<string, int32_t> uuid_first_replica; How about use KuduClient::Data::ListTablesWithInfo for this purpose? http://gerrit.cloudera.org:8080/#/c/18877/10/src/kudu/client/scan_token-test.cc@1595 PS10, Line 1595: std::map<string, int32_t> uuid_scan_count; Is it OK to judge it on metrics like scanner_*, that would simplify the code. -- 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: 10 Gerrit-Owner: Yuqi Du <[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, 05 Sep 2022 17:15:49 +0000 Gerrit-HasComments: Yes
