Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/18846 )
Change subject: [Client] Add query id to trace the whole query process ...................................................................... Patch Set 31: (22 comments) http://gerrit.cloudera.org:8080/#/c/18846/30//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18846/30//COMMIT_MSG@9 PS30, Line 9: is tra > is Done http://gerrit.cloudera.org:8080/#/c/18846/30//COMMIT_MSG@9 PS30, Line 9: an SQ > an SQL query Done http://gerrit.cloudera.org:8080/#/c/18846/30//COMMIT_MSG@10 PS30, Line 10: is > is Done http://gerrit.cloudera.org:8080/#/c/18846/30//COMMIT_MSG@11 PS30, Line 11: h > at Done http://gerrit.cloudera.org:8080/#/c/18846/30//COMMIT_MSG@11 PS30, Line 11: at differen > in a distributed manner Done http://gerrit.cloudera.org:8080/#/c/18846/30//COMMIT_MSG@13 PS30, Line 13: very execution unit. It is useful : in analysis of stuck, failed, and otherwise non-performa > It is useful in analysis of stuck, failed, and otherwise non-performant que Done http://gerrit.cloudera.org:8080/#/c/18846/30//COMMIT_MSG@18 PS30, Line 18: Scanning a Kudu table is split into multiple scan operations : on appropriate tablets, one scanner per tablet. When scanning : a Kudu table using Impala, corresponding scan nodes appear : for every scanner in the Impala's query pla > Scanning a Kudu table is split into multiple scan operations on appropriate Done http://gerrit.cloudera.org:8080/#/c/18846/30//COMMIT_MSG@24 PS30, Line 24: . We > originated from Done http://gerrit.cloudera.org:8080/#/c/18846/30//COMMIT_MSG@27 PS30, Line 27: : > with this patch it is now possible to post Impala's query identifier to Kud Done http://gerrit.cloudera.org:8080/#/c/18846/27/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/18846/27/src/kudu/client/client.h@3376 PS27, Line 3376: > Missed this comment? Done http://gerrit.cloudera.org:8080/#/c/18846/27/src/kudu/client/client.h@3377 PS27, Line 3377: ole process. > Missed this comment? Done http://gerrit.cloudera.org:8080/#/c/18846/27/src/kudu/client/client.h@3380 PS27, Line 3380: > Missed this comment? Done http://gerrit.cloudera.org:8080/#/c/18846/27/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/18846/27/src/kudu/client/client.cc@2019 PS27, Line 2019: } : : VLOG(2) << "Beginning " << data_->DebugString(); : > It seems this comment hasn't been answered (it was missed?). Sorry. I maybe not understand what you mean exactly. Could you give more detail? Here ,I just want to set a random query id when the user does not set the query id. Every query needs a unique query id even if the user does not set the query id. And Java client will be added the same functionality at the next commit. http://gerrit.cloudera.org:8080/#/c/18846/30/src/kudu/client/client.proto File src/kudu/client/client.proto: http://gerrit.cloudera.org:8080/#/c/18846/30/src/kudu/client/client.proto@201 PS30, Line 201: Identifier of a top-level query > How about: Done http://gerrit.cloudera.org:8080/#/c/18846/30/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: http://gerrit.cloudera.org:8080/#/c/18846/30/src/kudu/client/scan_token-test.cc@703 PS30, Line 703: replication factor. > nit: but the code below sets the replication factor to 1? Done http://gerrit.cloudera.org:8080/#/c/18846/30/src/kudu/client/scan_token-test.cc@709 PS30, Line 709: workload.Start(); : ASSERT_EVENTUALLY([&]() { ASSERT_GE > Is this crucial to customize these in the context of this test? If yes, pl Done http://gerrit.cloudera.org:8080/#/c/18846/30/src/kudu/client/scan_token-test.cc@729 PS30, Line 729: unique_ptr<KuduScanner> scanner; : ASSERT_OK(IntoUniqueScanner(client_.get(), *token, &scanner)); > Does it make sense to check for the 'query_id' field in scanner->data_->nex Good advise. A check will be added: ASSERT_EQ("query-id-for-test", scanner->data_->next_req_.query_id()); http://gerrit.cloudera.org:8080/#/c/18846/30/src/kudu/client/scan_token-test.cc@731 PS30, Line 731: ASSERT_OK(scanner->Open()); : ASSERT_EQ("query-id-for-test", scanner->data_->next_req_.query_id()); : KuduScanBatch batch; : ASSERT_TRUE(scanner->HasMoreRows()); : : > nit: maybe, check for the number of scanned rows to equal to workload.rows_ Done http://gerrit.cloudera.org:8080/#/c/18846/30/src/kudu/client/scan_token-test.cc@746 PS30, Line 746: vector<KuduScanToken*> tokens; : KuduScanTokenBuilder builder(table.get()); > Does it make sense to check for the 'query_id' field in scanner->data_->nex Good advise. A check will be added: ASSERT_FALSE(scanner->data_->next_req_.query_id().empty()) http://gerrit.cloudera.org:8080/#/c/18846/30/src/kudu/client/scan_token-test.cc@748 PS30, Line 748: ASSERT_OK(builder.Build(&tokens)); : : int64_t count = 0; : ASSERT_EQ(2, tokens.size()); : for (const auto& token : tokens) { : > nit: maybe, check for the number of rows to equal to workload.rows_inserted Done http://gerrit.cloudera.org:8080/#/c/18846/27/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/18846/27/src/kudu/tserver/tablet_server-test.cc@2870 PS27, Line 2870: next_seq_id++; : if (!resp.has_more_results()) { : VLOG(1) << "Finished scan"; : done = true; : } : } : } : }); : } : for (auto& t : threads) { : t.join(); : } : ASSERT_EQ(total_rows, kNumRows); : } : : // Test for a scan with query id in server side. : TEST_F(TabletServerTest, TestScanWithQueryId) { : InsertTestRowsDirect(0, 100); : : ScanRequestPB req; : > All right, but I don't see the code that would check for 'query_id_for_test Yes. At the beginning, query id was added to the LOG, but this solution was abandoned. And now query id is added to he EVENT TRACE. Here only checks a query with query id set in ScanRequestPB will be responsed with no errors. http://gerrit.cloudera.org:8080/#/c/18846/27/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/18846/27/src/kudu/tserver/tablet_service.cc@3039 PS27, Line 3039: size_t batch_size_bytes = GetMaxBatchSizeBytesHint(req); : > Uh-oh, I missed your response here, sorry. Thanks for your comments. -- 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: 31 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Jian Zhang <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Tue, 14 Feb 2023 05:03:30 +0000 Gerrit-HasComments: Yes
