Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20956 )
Change subject: IMPALA-12747: Atomic update of execution state ...................................................................... Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/20956/2/be/src/runtime/query-driver.cc File be/src/runtime/query-driver.cc: http://gerrit.cloudera.org:8080/#/c/20956/2/be/src/runtime/query-driver.cc@33 PS2, Line 33: DEFINE_string > DEFINE_string_hidden feels better for debugging purpose. This was previously defined in https://gerrit.cloudera.org/c/20956/2/be/src/service/impala-server.cc#b353. I moved it to provide better encapsulation around the results of running the planner, which is necessary to isolate the lifetime of exec_state. http://gerrit.cloudera.org:8080/#/c/20956/2/be/src/runtime/query-driver.cc@96 PS2, Line 96: ExecEnv::GetInstance()->frontend()->GetExecRequest(query_ctx, &exec_request))); > When SetExternalPlan() calls this function, the built-in planner will alway DoFrontendPlanning is only called from SetExternalPlan when the flag is set. So I'm not sure what you're getting at. http://gerrit.cloudera.org:8080/#/c/20956/2/be/src/runtime/query-driver.cc@99 PS2, Line 99: move > Is move required here? Or is it for performance reason? It's for performance. There's nothing preventing us from making a copy of exec_request, but it'd be wasteful. http://gerrit.cloudera.org:8080/#/c/20956/2/be/src/runtime/query-driver.cc@99 PS2, Line 99: exec_request_.reset(new TExecRequest(move(exec_request))); > This line of code does extra work when SetExternalPlan() calls this functio Yes, that's true, although it's not much work. I could have the function return the new TExecRequest via a pointer argument, and let the caller choose what to do with it. http://gerrit.cloudera.org:8080/#/c/20956/2/be/src/runtime/query-driver.cc@113 PS2, Line 113: const TQueryCtx& query_ctx, TExecRequest external_exec_request) { > Tracing back up the stack, external_exec_request needs to be passed by refe SetExternalPlan was previously a const&, so that can't have happened. Line 79 used to make a copy, I moved that to the call site and use it to modify the TExecRequest before I SetExecRequest. http://gerrit.cloudera.org:8080/#/c/20956/2/be/src/runtime/query-driver.cc@119 PS2, Line 119: RETURN_IF_ERROR(DoFrontendPlanning(query_ctx)); > Should this dump external_exec_request instead? That happens later at line 141. This dumps the plan that would have been generated, see https://gerrit.cloudera.org/c/20956/2/be/src/service/impala-server.cc#b1336. http://gerrit.cloudera.org:8080/#/c/20956/2/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/20956/2/be/src/service/client-request-state.cc@317 PS2, Line 317: default: > Handle TStmtType.UNKNOWN with distinct error message. Done http://gerrit.cloudera.org:8080/#/c/20956/2/bin/tsan-suppressions.txt File bin/tsan-suppressions.txt: http://gerrit.cloudera.org:8080/#/c/20956/2/bin/tsan-suppressions.txt@47 PS2, Line 47: : # TODO: IMPALA-12757: TSAN flags lock-order-inversion during internal-server-test > Mention the open issue link here. I'm actually not sure that's related. I'd rather track it in the ticket as well. http://gerrit.cloudera.org:8080/#/c/20956/2/common/thrift/Types.thrift File common/thrift/Types.thrift: http://gerrit.cloudera.org:8080/#/c/20956/2/common/thrift/Types.thrift@104 PS2, Line 104: UNKNOWN = 0 > Reordering enum has issue in the past Done -- To view, visit http://gerrit.cloudera.org:8080/20956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9a967c5c84b6a401f8f5764373f6cd7ee807545f Gerrit-Change-Number: 20956 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Fri, 26 Jan 2024 00:26:39 +0000 Gerrit-HasComments: Yes
