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/15 14:38:25 UTC
[kudu-CR] [Client] Add request id to trace the whole query process
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18846 )
Change subject: [Client] Add request id to trace the whole query process
......................................................................
Patch Set 17:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/18846/17/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18846/17/src/kudu/client/client-test.cc@1139
PS17, Line 1139: DoTestScanWithStringPredicate();
Need to check the request id is occured in the logs.
http://gerrit.cloudera.org:8080/#/c/18846/17/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/18846/17/src/kudu/client/client.h@2790
PS17, Line 2790: Add
nit: Set
http://gerrit.cloudera.org:8080/#/c/18846/17/src/kudu/client/client.h@2792
PS17, Line 2792: request
nit: request_id
http://gerrit.cloudera.org:8080/#/c/18846/17/src/kudu/client/client.h@2793
PS17, Line 2793: debug
debugging
http://gerrit.cloudera.org:8080/#/c/18846/17/src/kudu/client/client.h@3353
PS17, Line 3353: request
nit: request_id
http://gerrit.cloudera.org:8080/#/c/18846/17/src/kudu/client/client.h@3354
PS17, Line 3354: debug
nit: debugging
http://gerrit.cloudera.org:8080/#/c/18846/17/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/18846/17/src/kudu/client/client.cc@1735
PS17, Line 1735: if (request_id.empty()) {
: static ObjectIdGenerator oid_generator;
: data_->next_req_.set_request_id(oid_generator.Next());
Would it be better to use the random id in a upper layer, and set it regardlessly according to the parameter here?
http://gerrit.cloudera.org:8080/#/c/18846/17/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
http://gerrit.cloudera.org:8080/#/c/18846/17/src/kudu/client/scan_token-test.cc@450
PS17, Line 450: // Create schema
: KuduSchema schema;
: {
: KuduSchemaBuilder builder;
: builder.AddColumn("col")->NotNull()->Type(KuduColumnSchema::INT64)->PrimaryKey();
: ASSERT_OK(builder.Build(&schema));
: }
:
: // Create table
: shared_ptr<KuduTable> table;
: {
: unique_ptr<KuduPartialRow> split(schema.NewRow());
: ASSERT_OK(split->SetInt64("col", 0));
: unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
: #pragma GCC diagnostic push
: #pragma GCC diagnostic ignored "-Wdeprecated-declarations"
: ASSERT_OK(table_creator->table_name("table")
: .schema(&schema)
: .add_hash_partitions({ "col" }, 4)
: .split_rows({ split.release() })
: .num_replicas(1)
: .Create());
: #pragma GCC diagnostic pop
: ASSERT_OK(client_->OpenTable("table", &table));
: }
:
: // Create session
: shared_ptr<KuduSession> session = client_->NewSession();
: session->SetTimeoutMillis(10000);
: ASSERT_OK(session->SetFlushMode(KuduSession::AUTO_FLUSH_BACKGROUND));
:
: // Insert rows
: for (int i = -100; i < 100; i++) {
: unique_ptr<KuduInsert> insert(table->NewInsert());
: ASSERT_OK(insert->mutable_row()->SetInt64("col", i));
: ASSERT_OK(session->Apply(insert.release()));
: }
: ASSERT_OK(session->Flush());
:
How about use TestWorkload to simplify the code?
http://gerrit.cloudera.org:8080/#/c/18846/17/src/kudu/client/scan_token-test.cc@494
PS17, Line 494: ASSERT_OK(builder.SetBatchSizeBytes(0));
Not set it and keep the default value for it.
http://gerrit.cloudera.org:8080/#/c/18846/17/src/kudu/client/scan_token-test.cc@502
PS17, Line 502: ASSERT_EQ(0, scanner->data_->last_response_.data().num_rows());
What does it mean? Is it related to what you want to test?
And, have you test the request id functions works well or not?
http://gerrit.cloudera.org:8080/#/c/18846/17/src/kudu/client/scan_token-test.cc@510
PS17, Line 510: ASSERT_OK(builder.SetBatchSizeBytes(0));
Not set it and keep the default value for it.
http://gerrit.cloudera.org:8080/#/c/18846/17/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/18846/17/src/kudu/tserver/tablet_server-test.cc@2887
PS17, Line 2887: ASSERT_FALSE(resp.has_error());
Same, missing testing request_id occurred.
--
To view, visit http://gerrit.cloudera.org:8080/18846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9dbae801596726fec1c85ee547128da3179345d9
Gerrit-Change-Number: 18846
Gerrit-PatchSet: 17
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Sat, 15 Oct 2022 14:38:25 +0000
Gerrit-HasComments: Yes