Yingchun Lai 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 6: (13 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; : > Done Done http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/client-test.cc@6988 PS3, Line 6988: onst int other_ref_count(oth > If we do not set 'split_size_bytes', will use the meta data in the cache wi Done http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/client-test.cc@7010 PS3, Line 7010: ests that > I found duplicate data when I debug this case, and I'm not sure whether thi Yes, duplicate data is not acceptable. http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/client-test.cc@8603 PS6, Line 8603: Status BuildSchema() override { Is it necessary to create a diifferent schema with ClientTest? If not, it's no needed to override this function. http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/client-test.cc@8654 PS6, Line 8654: { : // Create session : shared_ptr<KuduSession> session = client_->NewSession(); : session->SetTimeoutMillis(10000); : ASSERT_OK(session->SetFlushMode(KuduSession::MANUAL_FLUSH)); : : // Should have no rows to begin with. : ASSERT_EQ(0, CountRowsFromClient(table.get())); : // Insert rows : string str_val; : str_val.assign(32 * 1024, '*'); : for (int i = 0; i < 1000; i++) { : unique_ptr<KuduInsert> insert(table->NewInsert()); : ASSERT_OK(insert->mutable_row()->SetInt32("key", i)); : ASSERT_OK(insert->mutable_row()->SetInt32("column1_i", i * 2)); : ASSERT_OK(insert->mutable_row()->SetInt32("column2_i", i * 3)); : ASSERT_OK(insert->mutable_row()->SetString("column3_s", str_val)); : ASSERT_OK(session->Apply(insert.release())); : ASSERT_OK(session->Flush()); : } : NO_FATALS(CheckNoRpcOverflow()); : } Can use InsertTestRows to insert data? http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/client-test.cc@8700 PS6, Line 8700: ASSERT_GE(tokens.size(), 4); The test should cover the case that return more tokens than tablet count, and cover cases SetSplitSizeBytes to 0, at least 2 different size less than tablet's size, to check that different token count is returned. http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/client.h@3359 PS6, Line 3359: splitSizeBytes Use the function name 'setSplitSizeBytes' would be better. http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/meta_cache.h File src/kudu/client/meta_cache.h: http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/meta_cache.h@412 PS6, Line 412: struct RemoteTabletWithID { It seems nobody use it? http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/meta_cache.h@446 PS6, Line 446: target_chunk_size_bytes nit: split_size_bytes http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/meta_cache.h@452 PS6, Line 452: std::vector<RangeWithRemoteTablet>* range_tablets, As shown in https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs, it’s recommend to put all input-only parameters before any output parameters. http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/meta_cache.h@453 PS6, Line 453: target_chunk_size_bytes nit: split_size_bytes, which is similar to Java client. http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/meta_cache.cc@1324 PS6, Line 1324: target_chunk_size_bytes nit: split_size_bytes http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/scan_token-internal.h File src/kudu/client/scan_token-internal.h: http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/scan_token-internal.h@93 PS6, Line 93: uint64_t target_chunk_size_bytes_ = 0; nit: same use split_size_bytes_ -- 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: 6 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Sun, 30 Oct 2022 12:25:55 +0000 Gerrit-HasComments: Yes
