You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yingchun Lai (Code Review)" <ge...@cloudera.org> on 2022/10/16 13:13:06 UTC
[kudu-CR] KUDU-3393 C++ client support split a tablet to mutil ranges and concurrent scan data
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