Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19517 )
Change subject: IMPALA-11898: Add query options in the profile event if the query failed in planning ...................................................................... Patch Set 3: (5 comments) Thanks for fixing this! http://gerrit.cloudera.org:8080/#/c/19517/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19517/1//COMMIT_MSG@13 PS1, Line 13: > I researched test_observability and still couldn't find a way to add test c test_error_query_state() is an example for testing failed profiles. http://gerrit.cloudera.org:8080/#/c/19517/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19517/3//COMMIT_MSG@7 PS3, Line 7: event "even" ? http://gerrit.cloudera.org:8080/#/c/19517/3//COMMIT_MSG@13 PS3, Line 13: Please mention the cause of the bug and how the query options are added. E.g. Currently, query options are added to profile in ClientRequestState::Exec() which is not executed if the query failed in planning or not admitted (e.g. timeout in queueing or cancelled before execution). This patch moves the logics to where the query options are ready to be added. To be specifit, "Query Options (set by configuration)" is there when the client submits the request, so we add it in the constructor of ClientRequestState. "Query Options (set by configuration and planner)" is ready when planning finishes. So it's moved to right after the call of RunFrontendPlanner(). http://gerrit.cloudera.org:8080/#/c/19517/3/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/19517/3/be/src/service/client-request-state.cc@228 PS3, Line 228: summary_profile_->AddInfoString("Query Type", PrintValue(stmt_type())); : if (query_ctx_.__isset.overridden_mt_dop_value) { : DCHECK(query_ctx_.client_request.query_options.__isset.mt_dop); : summary_profile_->AddInfoString("MT_DOP limited by admission control", : Substitute("Requested MT_DOP=$0 reduced to MT_DOP=$1", : query_ctx_.overridden_mt_dop_value, : query_ctx_.client_request.query_options.mt_dop)); : } I think we can also move these to right after the call of RunFrontendPlanner(). http://gerrit.cloudera.org:8080/#/c/19517/3/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/19517/3/be/src/service/impala-server.cc@1247 PS3, Line 1247: (*query_handle)->summary_profile()->AddInfoString( Plase add a comment, e.g. "Add profile info items that are ready after RunFrontendPlanner() finishes" I think we should move this into the above if-clause to ensure RunFrontendPlanner() is invoked. -- To view, visit http://gerrit.cloudera.org:8080/19517 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e9ce62008dd5b1671b09eda5365cbb0940ebe64 Gerrit-Change-Number: 19517 Gerrit-PatchSet: 3 Gerrit-Owner: Baike Xia <xiaba...@163.com> Gerrit-Reviewer: Baike Xia <xiaba...@163.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Comment-Date: Sun, 05 Mar 2023 00:01:46 +0000 Gerrit-HasComments: Yes
