Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/20282 )
Change subject: KUDU-3498 Scanner keeps alive in periodically ...................................................................... Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/20282/6/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/20282/6/src/kudu/client/client-test.cc@2973 PS6, Line 2973: InternalMiniClusterOptions options; : options.num_tablet_servers = 3; : cluster_->Shutdown(); : env_->DeleteRecursively(test_dir_); : cluster_.reset(new InternalMiniCluster(env_, options)); : ASSERT_OK(cluster_->Start()); : ASSERT_OK(KuduClientBuilder() : .add_master_server_addr(cluster_->mini_master()->bound_rpc_addr().ToString()) : .Build(&client_)); : : unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator()); : It would be great to do these prepare and clean up work in gtest's SetUp() and TearDown() functions. http://gerrit.cloudera.org:8080/#/c/20282/6/src/kudu/client/client-test.cc@2985 PS6, Line 2985: unique_ptr<KuduPartialRow> lower_bound1(schema_.NewRow()); : ASSERT_OK(lower_bound1->SetInt32("key", INT32_MIN)); : unique_ptr<KuduPartialRow> upper_bound1(schema_.NewRow()); : ASSERT_OK(upper_bound1->SetInt32("key", 1000)); : table_creator->add_range_partition(lower_bound1.release(), upper_bound1.release(), : KuduTableCreator::EXCLUSIVE_BOUND, : KuduTableCreator::INCLUSIVE_BOUND); : : unique_ptr<KuduPartialRow> lower_bound2(schema_.NewRow()); : ASSERT_OK(lower_bound2->SetInt32("key", 1000)); : unique_ptr<KuduPartialRow> upper_bound2(schema_.NewRow()); : ASSERT_OK(upper_bound2->SetInt32("key", 2000)); : table_creator->add_range_partition(lower_bound2.release(), upper_bound2.release(), : KuduTableCreator::EXCLUSIVE_BOUND, : KuduTableCreator::INCLUSIVE_BOUND); : : unique_ptr<KuduPartialRow> lower_bound3(schema_.NewRow()); : ASSERT_OK(lower_bound3->SetInt32("key", 2000)); : unique_ptr<KuduPartialRow> upper_bound3(schema_.NewRow()); : ASSERT_OK(upper_bound3->SetInt32("key", INT32_MAX)); : table_creator->add_range_partition(lower_bound3.release(), upper_bound3.release(), : KuduTableCreator::EXCLUSIVE_BOUND, : KuduTableCreator::INCLUSIVE_BOUND); Is it enough to create a table just with hash partitions? http://gerrit.cloudera.org:8080/#/c/20282/6/src/kudu/client/client-test.cc@3010 PS6, Line 3010: ASSERT_OK(table_creator->table_name(kTableName) : .schema(&schema_) : .num_replicas(3) : .set_range_partition_columns({ "key" }) : .timeout(MonoDelta::FromSeconds(60)) : .Create()); How about using CreateTable() in ClientTest to simplify the code? http://gerrit.cloudera.org:8080/#/c/20282/6/src/kudu/client/client-test.cc@3051 PS6, Line 3051: if (!ContainsKey(scan_num_by_tablet_id, cur_tablet_id)) { : scan_num_by_tablet_id.emplace(cur_tablet_id, 1); : } else { : int count = scan_num_by_tablet_id[cur_tablet_id]; : scan_num_by_tablet_id[cur_tablet_id] = ++count; : } How about: auto& scan_num = LookupOrInsert(&scan_num_by_tablet_id, scan_num_by_tablet_id, 0); scan_num++; http://gerrit.cloudera.org:8080/#/c/20282/6/src/kudu/client/client-test.cc@3080 PS6, Line 3080: Test case 2 Sepate the test in another test suite. http://gerrit.cloudera.org:8080/#/c/20282/6/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/20282/6/src/kudu/client/client.h@2958 PS6, Line 2958: /// @return It returns a non-OK if building a messeger failed or : /// the scanner is not opened. nit: indent /// @return It returns a non-OK if building a messeger failed or /// the scanner is not opened. http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/scanner-internal.cc@115 PS4, Line 115: Start > Function KeepAlive() had done this check. And KeepAlivePeriodically() calls Although the KeepAlive() can deal with these conditions, but the PeriodicTimer internal timer is continously been called, it may cost some resource, it would be great to early stop the timer rather than wait until the destructor. -- To view, visit http://gerrit.cloudera.org:8080/20282 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1165d96814eb4bcd5db9b5cb60403fffc5b18c81 Gerrit-Change-Number: 20282 Gerrit-PatchSet: 6 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Wed, 20 Sep 2023 06:44:54 +0000 Gerrit-HasComments: Yes
