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 <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Sun, 16 Oct 2022 13:13:06 +0000 Gerrit-HasComments: Yes
