KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18945 )
Change subject: KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data ...................................................................... Patch Set 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/client-test.cc@6922 PS3, Line 6922: ASSERT_TRUE(it == it_next); : } : : { : KuduScanBatch::const_iterator it_end = batch.begin(); : std::advance(it_end, kRowNum); : ASSERT_TRUE(batch.end() == it_end); : } : : { : KuduScanBatch::const_iterator it(batch.begin()); : ASSERT_TRUE(it++ == batch.begin()); : : KuduScanBatch::const_iterator it_next(batch.begin()); : ASSERT_TRUE(++it_next == it); : } : : // Check the prefix increment iterator. : { : int count = 0; : for (KuduScanBatch::const_iterator it = batch.begin(); : it != batch.end(); ++it) { : ++count; : } : CHECK_EQ(ref_count, count); : } : : // Check the postfix increment iterator. : { : int count = 0; : for (KuduScanBatch::const_iterator it = batch.begin(); : it != batch.end(); it++) { : ++count; : } : CHECK_EQ(ref_count, count); : } : : // Check access to row via indirection operator * : // and member access through pointer operator -> : { : KuduScanBatch::const_iterator it(batch.begin()); : ASSERT_TRUE(it != batch.end()); : int32_t x = 0, y = 0; : ASSERT_OK((*it).GetInt32(0, &x)); : ASSERT_OK(it->GetInt32(0, &y)); : ASSERT_EQ(x, y); : } : : { : KuduScanBatch::const_iterator it_pre(batch.begin()); : KuduScanBatch::const_iterator it_post(batch.begin()); : for (; it_pre != batch.end(); ++it_pre, it_post++) { : ASSERT_TRUE(it_pre == it_post); : ASSERT_FALSE(it_pre != it_post); : } : } : : // Check that iterators which are going over different batches : // are different, even if they iterate over the same raw data. : { : KuduScanner other_scanner(client_table_.get()); : ASSERT_OK(other_scanner.Open()); : : KuduScanBatch other_batch; : > To prepare the test table, we only need to create a table with 4 partitions Do you mean I need to add this case to the integration test? But I think this is just a simple function verification, it is not meaningful to add it to the integration test just to simplify the creation of tables http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/client.h@3359 PS3, Line 3359: /// It's corresponding to 'splitSizeBytes' in Java client. > Is it corresponding to 'splitSizeBytes' in Java client? It would better to Done http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/meta_cache.cc@1335 PS3, Line 1335: RETURN_NOT_OK(sync.Wait()); : > The variable 's' seems not used any more, how about just RETURN_NOT_OK(syn Done http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/meta_cache.cc@1343 PS3, Line 1343: OK(); > How about use emplace_back? Done http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/meta_cache.cc@1346 PS3, Line 1346: > nit: add a DCHECK_GT(target_chunk_size_bytes, 0) ? Done http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/meta_cache.cc@1350 PS3, Line 1350: RETURN_NOT_OK(table->client()->data_->GetTabletServer(table->client(), : tablet, : KuduClient::LEADER_ONLY, : blacklist, : &candidates, : &ts)); : CHECK(ts); > How about just RETURN_NOT_OK(table->client()->data_->GetTabletServer(...)) Done http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/meta_cache.cc@1357 PS3, Line 1357: CHECK(ts->proxy()); > CHECK ts is not nullptr at first? Done http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/meta_cache.cc@1381 PS3, Line 1381: RETURN_NOT_OK(proxy->SplitKeyRange(req, &resp, &rpc)); : if (resp.has_erro > How about just RETURN_NOT_OK(proxy->SplitKeyRange(...)); ? Done http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/meta_cache.cc@1392 PS3, Line 1392: > How about use emplace_back? Done -- To view, visit http://gerrit.cloudera.org:8080/18945 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272 Gerrit-Change-Number: 18945 Gerrit-PatchSet: 4 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Wed, 19 Oct 2022 06:13:18 +0000 Gerrit-HasComments: Yes
