Yuqi Du 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 11: (5 comments) I have fix some crs and reply others, thanks for your crs and time. 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: RETURN_NOT_ > Why just warn but not return error? done . RETURN_NOT_OK is fine. 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: // Create a table with the specified name and schema with replication factor > How about separete into 2 parameters explicitly? two int is not adviced by tidy, so add a struct for this. restore it, because create table by TestWorkload at other crs. http://gerrit.cloudera.org:8080/#/c/18877/10/src/kudu/client/scan_token-test.cc@1515 PS10, Line 1515: ASSERT_EVENTUALLY([&]() { : ASSERT_GE(workload.rows_inserted(), kCount); : }); : workload.StopAndJoin(); : } : shared_ptr<KuduTable> table; : ASSERT_OK(cluster_->CreateClient(nullptr, &client_)); : ASSERT_OK(client_->OpenTable(kTableName, &table)); : ASSERT_NE(nullptr, table.get()); : : std::map<string, int32_t> uuid_leaders; : std::vector<scoped_refptr<master::TableInfo>> tables; : CatalogManager* catalog_manager = cluster_->mini_master()->master()->catalog_manager(); : { : CatalogManager::ScopedLeaderSharedLock leader_lock(catalog_manager); : CHECK_OK(leader_lock.first_failed_status()); : > Is it OK to use TestWorkload to populate data? ok http://gerrit.cloudera.org:8080/#/c/18877/10/src/kudu/client/scan_token-test.cc@1542 PS10, Line 1542: for (auto& tablet : tablets) { : master::TabletMetadataLock tablet_l(tab > How about use KuduClient::Data::ListTablesWithInfo for this purpose? A good function, I look it and find It seems not get the infos. The map is the amount of first replica located in permanent_uuid, but the function no return the infos and at the same time no info about tablet's role. http://gerrit.cloudera.org:8080/#/c/18877/10/src/kudu/client/scan_token-test.cc@1595 PS10, Line 1595: {cluster_->mini_tablet_server(i)->se > Is it OK to judge it on metrics like scanner_*, that would simplify the cod I find some metrics. These metrics seems not good enough to do the unit test. as this: src/kudu/client/predicate-test.cc:69:METRIC_DECLARE_counter(scanner_predicates_disabled); src/kudu/tablet/tablet_metrics.cc:82:METRIC_DEFINE_counter(tablet, scanner_rows_returned, "Scanner Rows Returned", src/kudu/tablet/tablet_metrics.cc:88:METRIC_DEFINE_counter(tablet, scanner_cells_returned, "Scanner Cells Returned", src/kudu/tablet/tablet_metrics.cc:94:METRIC_DEFINE_counter(tablet, scanner_bytes_returned, "Scanner Bytes Returned", src/kudu/tablet/tablet_metrics.cc:101:METRIC_DEFINE_counter(tablet, scanner_predicates_disabled, "Scanner Column Predicates Disabled", src/kudu/tablet/tablet_metrics.cc:109:METRIC_DEFINE_counter(tablet, scanner_rows_scanned, "Scanner Rows Scanned", src/kudu/tablet/tablet_metrics.cc:118:METRIC_DEFINE_counter(tablet, scanner_cells_scanned_from_disk, "Scanner Cells Scanned From Disk", src/kudu/tablet/tablet_metrics.cc:130:METRIC_DEFINE_counter(tablet, scanner_bytes_scanned_from_disk, "Scanner Bytes Scanned From Disk", src/kudu/tserver/scanner_metrics.cc:28:METRIC_DEFINE_histogram(server, scanner_duration, -- 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: 11 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: Fri, 09 Sep 2022 10:26:28 +0000 Gerrit-HasComments: Yes
