You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "KeDeng (Code Review)" <ge...@cloudera.org> on 2022/09/07 08:57:06 UTC
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
KeDeng has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18945
Change subject: KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
......................................................................
KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
---
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
4 files changed, 186 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18945/1
--
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: newchange
Gerrit-Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
Gerrit-Change-Number: 18945
Gerrit-PatchSet: 1
Gerrit-Owner: KeDeng <kd...@gmail.com>
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
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 5:
(5 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@439
PS3, Line 439: string buf;
> It's recommend to use ASSERT_* instead of CHECK_* in unit tests to avoid te
Done
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;
:
> OK. Besides using TestWorkload, is the default table created by SetUp() can
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
> I'm not sure if I get your point, could you please describe more clearly ab
If we do not set 'split_size_bytes', will use the meta data in the cache without send rpc, that is the situation before modification.
We will send rpc to tservers to get the split key ranges if we set the 'split_size_bytes'.
http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/client-test.cc@7010
PS3, Line 7010: ests that
> Why it's ASSERT_GE but not ASSERT_EQ?
I found duplicate data when I debug this case, and I'm not sure whether this is a fault. I will do more tests to check it.
http://gerrit.cloudera.org:8080/#/c/18945/4/src/kudu/client/scan_token-internal.h
File src/kudu/client/scan_token-internal.h:
http://gerrit.cloudera.org:8080/#/c/18945/4/src/kudu/client/scan_token-internal.h@85
PS4, Line 85: void SplitSizeBytes(uint64_t split_size_bytes) {
> How about using 'SplitSizeBytes' too?
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: 5
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 25 Oct 2022 11:51:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Yingchun Lai, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18945
to look at the new patch set (#12).
Change subject: KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
......................................................................
KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
I add a param for build 'KuduScanToken', like this :
`
KuduScanTokenBuilder builder(table);
vector<KuduScanToken*> tokens;
ElementDeleter deleter(&tokens);
// set splitSizeBytes
builder.SetSplitSizeBytes(1000);
ASSERT_OK(builder.Build(&tokens));
`
The default value of split_size_bytes is 0, and this means we don't split the key range
for a tablet.
If the value of split_size_bytes is nonzero, we will try to send a SplitKeyRangeRPC to
tservers. We may get more tokens than tablets num and the more tokens will help us scan faster.
Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
7 files changed, 402 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18945/12
--
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: newpatchset
Gerrit-Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
Gerrit-Change-Number: 18945
Gerrit-PatchSet: 12
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Yingchun Lai, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18945
to look at the new patch set (#11).
Change subject: KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
......................................................................
KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
I add a param for build 'KuduScanToken', like this :
`
KuduScanTokenBuilder builder(table);
vector<KuduScanToken*> tokens;
ElementDeleter deleter(&tokens);
// set splitSizeBytes
builder.SetSplitSizeBytes(1000);
ASSERT_OK(builder.Build(&tokens));
`
The default value of split_size_bytes is 0, and this means we don't split the key range
for a tablet.
If the value of split_size_bytes is nonzero, we will try to send a SplitKeyRangeRPC to
tservers. We may get more tokens than tablets num and the more tokens will help us scan faster.
Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
7 files changed, 399 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18945/11
--
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: newpatchset
Gerrit-Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
Gerrit-Change-Number: 18945
Gerrit-PatchSet: 11
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18945
to look at the new patch set (#3).
Change subject: KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
......................................................................
KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
I add a param for build 'KuduScanToken', like this :
`
KuduScanTokenBuilder builder(table);
vector<KuduScanToken*> tokens;
ElementDeleter deleter(&tokens);
// set target_chunk_size_bytes
builder.SetTargetChunkSizeBytes(1000);
ASSERT_OK(builder.Build(&tokens));
`
The default value of target_chunk_size_bytes is 0, and this means we don't split the key range
for a tablet.
If the value of 'target_chunk_size_bytes' is nonzero, we will try to send a SplitKeyRangeRPC to
tservers. We may get more tokens than tablets num and the more tokens will help us scan faster.
Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
7 files changed, 340 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18945/3
--
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: newpatchset
Gerrit-Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
Gerrit-Change-Number: 18945
Gerrit-PatchSet: 3
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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 3:
(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: // Set a low flush threshold so we can scan a mix of flushed data in
: // in-memory data.
: FLAGS_flush_threshold_mb = 1;
: FLAGS_flush_threshold_secs = 1;
: // Disable rowset compact to prevent DRSs being merged because they are too small.
: FLAGS_enable_rowset_compaction = false;
:
: // build schema
: KuduSchemaBuilder b;
: KuduSchema schema;
: {
: b.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
: b.AddColumn("column1_i")->Type(KuduColumnSchema::INT32)->NotNull()
: ->Default(KuduValue::FromInt(123));
: b.AddColumn("column2_i")->Type(KuduColumnSchema::INT32)->NotNull()
: ->Default(KuduValue::FromInt(456));
: b.AddColumn("column3_s")->Type(KuduColumnSchema::STRING)->Nullable()
: ->Default(KuduValue::CopyString("test"));
: b.AddColumn("non_null_with_default")->Type(KuduColumnSchema::INT32)->NotNull()
: ->Default(KuduValue::FromInt(123456));
: }
: ASSERT_OK(b.Build(&schema));
:
: // build table
: const string table_name = "TestGetTableKeyRange";
: unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
: vector<int> keys = { 0, 250, 500, 750 };
: for (int i = 0; i < keys.size(); i++) {
: unique_ptr<KuduPartialRow> lower_bound(schema.NewRow());
: ASSERT_OK(lower_bound->SetInt32("key", keys[i]));
: unique_ptr<KuduPartialRow> upper_bound(schema.NewRow());
: ASSERT_OK(upper_bound->SetInt32("key", keys[i] + 250));
: table_creator->add_range_partition(lower_bound.release(), upper_bound.release());
: }
: ASSERT_OK(table_creator->table_name(table_name)
: .schema(&schema)
: .num_replicas(1)
: .set_range_partition_columns({ "key" })
: .Create());
: client::sp::shared_ptr<KuduTable> table;
: ASSERT_OK(client_->OpenTable(table_name, &table));
: {
: // Create session
: shared_ptr<KuduSession> session = client_->NewSession();
: session->SetTimeoutMillis(10000);
: ASSERT_OK(session->SetFlushMode(KuduSession::MANUAL_FLUSH));
: //ASSERT_OK(session->SetFlushMode(KuduSession::AUTO_FLUSH_BACKGROUND));
:
: // 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());
: }
:
To prepare the test table, we only need to create a table with 4 partitions, and fill the table with some data, each tablet has data larger than 50 bytes and less than 1 GiB, right? Could it be simplified to use TestWorkload fot this?
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: void SetTargetChunkSizeBytes(uint64_t target_chunk_size_bytes);
Is it corresponding to 'splitSizeBytes' in Java client? It would better to use the same name if it is, it would be easier for users to accept the same concept with the same name.
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: Status s = sync.Wait();
: RETURN_NOT_OK(s);
The variable 's' seems not used any more, how about just RETURN_NOT_OK(sync.Wait()); ?
http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/meta_cache.cc@1343
PS3, Line 1343: push_back
How about use emplace_back?
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) ?
http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/meta_cache.cc@1350
PS3, Line 1350: s = table->client()->data_->GetTabletServer(table->client(),
: tablet,
: KuduClient::LEADER_ONLY,
: blacklist,
: &candidates,
: &ts);
: RETURN_NOT_OK(s);
How about just RETURN_NOT_OK(table->client()->data_->GetTabletServer(...)); ?
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?
http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/meta_cache.cc@1381
PS3, Line 1381: s = proxy->SplitKeyRange(req, &resp, &rpc);
: RETURN_NOT_OK(s);
How about just RETURN_NOT_OK(proxy->SplitKeyRange(...)); ?
http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/meta_cache.cc@1392
PS3, Line 1392: push_back
How about use emplace_back?
--
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: 3
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Sun, 16 Oct 2022 13:13:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
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 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18945/8/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18945/8/src/kudu/client/client-test.cc@8739
PS8, Line 8739: builder.SetSplitSizeBytes(20);
> SetSplitSizeBytes as 20, you can insert plenty of data to make splitSizeByt
Yes, you're right. I've change it in the latest version and disable rowset compact by --enable_rowset_compaction.
I'm sorry for forget update the reply.
--
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: 12
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 10 Nov 2022 06:42:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Yingchun Lai, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18945
to look at the new patch set (#9).
Change subject: KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
......................................................................
KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
I add a param for build 'KuduScanToken', like this :
`
KuduScanTokenBuilder builder(table);
vector<KuduScanToken*> tokens;
ElementDeleter deleter(&tokens);
// set splitSizeBytes
builder.SetSplitSizeBytes(1000);
ASSERT_OK(builder.Build(&tokens));
`
The default value of split_size_bytes is 0, and this means we don't split the key range
for a tablet.
If the value of split_size_bytes is nonzero, we will try to send a SplitKeyRangeRPC to
tservers. We may get more tokens than tablets num and the more tokens will help us scan faster.
Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
7 files changed, 400 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18945/9
--
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: newpatchset
Gerrit-Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
Gerrit-Change-Number: 18945
Gerrit-PatchSet: 9
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Yingchun Lai, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18945
to look at the new patch set (#6).
Change subject: KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
......................................................................
KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
I add a param for build 'KuduScanToken', like this :
`
KuduScanTokenBuilder builder(table);
vector<KuduScanToken*> tokens;
ElementDeleter deleter(&tokens);
// set splitSizeBytes
builder.SetSplitSizeBytes(1000);
ASSERT_OK(builder.Build(&tokens));
`
The default value of split_size_bytes is 0, and this means we don't split the key range
for a tablet.
If the value of split_size_bytes is nonzero, we will try to send a SplitKeyRangeRPC to
tservers. We may get more tokens than tablets num and the more tokens will help us scan faster.
Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
7 files changed, 357 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18945/6
--
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: newpatchset
Gerrit-Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
Gerrit-Change-Number: 18945
Gerrit-PatchSet: 6
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Yingchun Lai, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18945
to look at the new patch set (#10).
Change subject: KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
......................................................................
KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
I add a param for build 'KuduScanToken', like this :
`
KuduScanTokenBuilder builder(table);
vector<KuduScanToken*> tokens;
ElementDeleter deleter(&tokens);
// set splitSizeBytes
builder.SetSplitSizeBytes(1000);
ASSERT_OK(builder.Build(&tokens));
`
The default value of split_size_bytes is 0, and this means we don't split the key range
for a tablet.
If the value of split_size_bytes is nonzero, we will try to send a SplitKeyRangeRPC to
tservers. We may get more tokens than tablets num and the more tokens will help us scan faster.
Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
7 files changed, 400 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18945/10
--
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: newpatchset
Gerrit-Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
Gerrit-Change-Number: 18945
Gerrit-PatchSet: 10
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18945/8/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18945/8/src/kudu/client/client-test.cc@8739
PS8, Line 8739: builder.SetSplitSizeBytes(20);
> With the influence of gc and other behaviors? the token nums we get may dif
SetSplitSizeBytes as 20, you can insert plenty of data to make splitSizeBytes is far less than tablet size, and you can disable compact/gc by setting flag enable_maintenance_manager=false.
ASSERT_NE(token_size_a, token_size_b) is not enough, if token_size_a and token_size_b are possible equal to 4, the assertion may fail.
--
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: 12
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 10 Nov 2022 04:13:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Yingchun Lai, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18945
to look at the new patch set (#7).
Change subject: KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
......................................................................
KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
I add a param for build 'KuduScanToken', like this :
`
KuduScanTokenBuilder builder(table);
vector<KuduScanToken*> tokens;
ElementDeleter deleter(&tokens);
// set splitSizeBytes
builder.SetSplitSizeBytes(1000);
ASSERT_OK(builder.Build(&tokens));
`
The default value of split_size_bytes is 0, and this means we don't split the key range
for a tablet.
If the value of split_size_bytes is nonzero, we will try to send a SplitKeyRangeRPC to
tservers. We may get more tokens than tablets num and the more tokens will help us scan faster.
Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
7 files changed, 392 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18945/7
--
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: newpatchset
Gerrit-Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
Gerrit-Change-Number: 18945
Gerrit-PatchSet: 7
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Yingchun Lai, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18945
to look at the new patch set (#8).
Change subject: KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
......................................................................
KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
I add a param for build 'KuduScanToken', like this :
`
KuduScanTokenBuilder builder(table);
vector<KuduScanToken*> tokens;
ElementDeleter deleter(&tokens);
// set splitSizeBytes
builder.SetSplitSizeBytes(1000);
ASSERT_OK(builder.Build(&tokens));
`
The default value of split_size_bytes is 0, and this means we don't split the key range
for a tablet.
If the value of split_size_bytes is nonzero, we will try to send a SplitKeyRangeRPC to
tservers. We may get more tokens than tablets num and the more tokens will help us scan faster.
Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
7 files changed, 399 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18945/8
--
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: newpatchset
Gerrit-Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
Gerrit-Change-Number: 18945
Gerrit-PatchSet: 8
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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 4:
(5 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@439
PS3, Line 439: string buf;
It's recommend to use ASSERT_* instead of CHECK_* in unit tests to avoid tests crash.
The same to the other places.
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;
:
> Do you mean I need to add this case to the integration test? But I think th
OK. Besides using TestWorkload, is the default table created by SetUp() can be used? You can do a small refactor and/or add a new class inherit from ClientTest and override GenerateSplitRows() maybe.
http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/client-test.cc@6988
PS3, Line 6988: onst int other_ref_count(oth
I'm not sure if I get your point, could you please describe more clearly about what cases do these tests want to check? thanks!
http://gerrit.cloudera.org:8080/#/c/18945/3/src/kudu/client/client-test.cc@7010
PS3, Line 7010: ests that
Why it's ASSERT_GE but not ASSERT_EQ?
http://gerrit.cloudera.org:8080/#/c/18945/4/src/kudu/client/scan_token-internal.h
File src/kudu/client/scan_token-internal.h:
http://gerrit.cloudera.org:8080/#/c/18945/4/src/kudu/client/scan_token-internal.h@85
PS4, Line 85: void TargetChunkSizeBytes(uint64_t target_chunk_size_bytes) {
How about using 'SplitSizeBytes' too?
--
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 <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Sat, 22 Oct 2022 16:38:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18945
to look at the new patch set (#2).
Change subject: KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
......................................................................
KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
I add a param for build 'KuduScanToken', like this :
`
KuduScanTokenBuilder builder(table);
vector<KuduScanToken*> tokens;
ElementDeleter deleter(&tokens);
// set target_chunk_size_bytes
builder.SetTargetChunkSizeBytes(1000);
ASSERT_OK(builder.Build(&tokens));
`
The default value of target_chunk_size_bytes is 0, and this means we don't split the key range
for a tablet.
If the value of 'target_chunk_size_bytes' is nonzero, we will try to send a SplitKeyRangeRPC to
tservers. We may get more tokens than tablets num and the more tokens will help us scan faster.
Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
7 files changed, 340 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18945/2
--
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: newpatchset
Gerrit-Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
Gerrit-Change-Number: 18945
Gerrit-PatchSet: 2
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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 <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Sun, 30 Oct 2022 12:25:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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 12: Code-Review+2
--
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: 12
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 10 Nov 2022 06:57:44 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
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 <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Wed, 19 Oct 2022 06:13:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Yingchun Lai, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18945
to look at the new patch set (#4).
Change subject: KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
......................................................................
KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
I add a param for build 'KuduScanToken', like this :
`
KuduScanTokenBuilder builder(table);
vector<KuduScanToken*> tokens;
ElementDeleter deleter(&tokens);
// set splitSizeBytes
builder.SetSplitSizeBytes(1000);
ASSERT_OK(builder.Build(&tokens));
`
The default value of split_size_bytes is 0, and this means we don't split the key range
for a tablet.
If the value of split_size_bytes is nonzero, we will try to send a SplitKeyRangeRPC to
tservers. We may get more tokens than tablets num and the more tokens will help us scan faster.
Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
7 files changed, 345 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18945/4
--
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: newpatchset
Gerrit-Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
Gerrit-Change-Number: 18945
Gerrit-PatchSet: 4
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Yingchun Lai, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18945
to look at the new patch set (#5).
Change subject: KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
......................................................................
KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
I add a param for build 'KuduScanToken', like this :
`
KuduScanTokenBuilder builder(table);
vector<KuduScanToken*> tokens;
ElementDeleter deleter(&tokens);
// set splitSizeBytes
builder.SetSplitSizeBytes(1000);
ASSERT_OK(builder.Build(&tokens));
`
The default value of split_size_bytes is 0, and this means we don't split the key range
for a tablet.
If the value of split_size_bytes is nonzero, we will try to send a SplitKeyRangeRPC to
tservers. We may get more tokens than tablets num and the more tokens will help us scan faster.
Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
7 files changed, 357 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18945/5
--
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: newpatchset
Gerrit-Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
Gerrit-Change-Number: 18945
Gerrit-PatchSet: 5
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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 8:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18945/8/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18945/8/src/kudu/client/client-test.cc@8637
PS8, Line 8637: , int diff_value = 120
There is only one caller, and it's always 120, how about set it as a local const variable?
http://gerrit.cloudera.org:8080/#/c/18945/8/src/kudu/client/client-test.cc@8649
PS8, Line 8649: diff_value
Could you please add some comments to describe what's the purpose of diff_value?
http://gerrit.cloudera.org:8080/#/c/18945/8/src/kudu/client/client-test.cc@8739
PS8, Line 8739: ASSERT_LE(4, token_size_b);
Is it definite that 4 < token_size_b (and 4 < token_size_a), if it is, better to use ASSERT_LT than ASSERT_LE.
If token_size_a == token_size_b == 4, the test will success, but it's not what you want I guess.
--
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: 8
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Sun, 06 Nov 2022 12:56:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
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 9:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18945/8/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18945/8/src/kudu/client/client-test.cc@8637
PS8, Line 8637: ) {
> There is only one caller, and it's always 120, how about set it as a local
Done
http://gerrit.cloudera.org:8080/#/c/18945/8/src/kudu/client/client-test.cc@8649
PS8, Line 8649:
> Could you please add some comments to describe what's the purpose of diff_v
Done
http://gerrit.cloudera.org:8080/#/c/18945/8/src/kudu/client/client-test.cc@8739
PS8, Line 8739: token_size_b = tokens.size();
> Is it definite that 4 < token_size_b (and 4 < token_size_a), if it is, bett
With the influence of gc and other behaviors? the token nums we get may different, that means different splitSizeBytes leads to different tokens and the min token size is the tablets num. And I check the different token size in line 8746.
--
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: 9
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 07 Nov 2022 02:17:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
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 7:
(10 comments)
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: KuduSchemaBuilder b;
> Is it necessary to create a diifferent schema with ClientTest? If not, it's
Done
http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/client-test.cc@8654
PS6, Line 8654: ASSERT_OK(session->Apply(insert.release()));
: ASSERT_OK(session->Flush());
: }
: }
: }
:
: protected:
: static constexpr const char* const kTableName = "client-testrange";
:
: shared_ptr<KuduTable> range_table_;
: };
:
: TEST_F(TableKeyRangeTest, TestGetTableKeyRange) {
: client::sp::shared_ptr<KuduTable> table;
: ASSERT_OK(client_->OpenTable(kTableName, &table));
: {
: // Create session
: shared_ptr<KuduSession> session = client_->NewSession();
: session->SetTimeoutMillis(10000);
: ASSERT_OK(session->SetFlushMode(KuduSession::MANUAL_FLUSH));
:
:
> Can use InsertTestRows to insert data?
Done
http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/client-test.cc@8700
PS6, Line 8700: // If the splitSizeBytes set to 0 , we search the meta cache.
> The test should cover the case that return more tokens than tablet count, a
Done
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: setSplitSizeBy
> Use the function name 'setSplitSizeBytes' would be better.
Done
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 RangeWithRemoteTablet {
> It seems nobody use it?
Done
http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/meta_cache.h@446
PS6, Line 446: Looku
> nit: split_size_bytes
Done
http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/meta_cache.h@452
PS6, Line 452: // 'remote_tablet' if not null, and calling 'lookup_complete_cb' once the
> As shown in https://google.github.io/styleguide/cppguide.html#Inputs_and_Ou
Done
http://gerrit.cloudera.org:8080/#/c/18945/6/src/kudu/client/meta_cache.h@453
PS6, Line 453: s with non-failed LEADE
> nit: split_size_bytes, which is similar to Java client.
Done
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: oDelta& timeout,
> nit: split_size_bytes
Done
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 split_size_bytes_ = 0;
> nit: same use split_size_bytes_
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: 7
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Fri, 04 Nov 2022 08:19:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18945 )
Change subject: KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
......................................................................
KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
I add a param for build 'KuduScanToken', like this :
`
KuduScanTokenBuilder builder(table);
vector<KuduScanToken*> tokens;
ElementDeleter deleter(&tokens);
// set splitSizeBytes
builder.SetSplitSizeBytes(1000);
ASSERT_OK(builder.Build(&tokens));
`
The default value of split_size_bytes is 0, and this means we don't split the key range
for a tablet.
If the value of split_size_bytes is nonzero, we will try to send a SplitKeyRangeRPC to
tservers. We may get more tokens than tablets num and the more tokens will help us scan faster.
Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
Reviewed-on: http://gerrit.cloudera.org:8080/18945
Tested-by: Kudu Jenkins
Reviewed-by: Yingchun Lai <ac...@gmail.com>
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
7 files changed, 402 insertions(+), 25 deletions(-)
Approvals:
Kudu Jenkins: Verified
Yingchun Lai: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: I207f9584cd558d32fcd9e8de7d6c25e517377272
Gerrit-Change-Number: 18945
Gerrit-PatchSet: 13
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>