Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/17104 )
Change subject: IMPALA-10535: Add interface to ImpalaServer for execution of externally compiled statements ...................................................................... Patch Set 7: (13 comments) http://gerrit.cloudera.org:8080/#/c/17104/7/be/src/runtime/query-driver.cc File be/src/runtime/query-driver.cc: http://gerrit.cloudera.org:8080/#/c/17104/7/be/src/runtime/query-driver.cc@67 PS7, Line 67: exec_request > Nit: I think it would be clearer to call this 'external_exec_request' Done http://gerrit.cloudera.org:8080/#/c/17104/7/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: http://gerrit.cloudera.org:8080/#/c/17104/7/be/src/service/impala-hs2-server.cc@458 PS7, Line 458: execRequest > nit: exec_request (here and several other places in this patch) Done http://gerrit.cloudera.org:8080/#/c/17104/7/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/17104/7/be/src/service/impala-server.h@678 PS7, Line 678: /// have been checked out. > comment might benefit from a brief mention of what 'exec_request' is Done http://gerrit.cloudera.org:8080/#/c/17104/7/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/17104/7/be/src/service/impala-server.cc@319 PS7, Line 319: "TExecRequest-{internal|external}.{query_id.hi}-{query_id.lo}"); > might be worth explicitly saying this is for debugging Done http://gerrit.cloudera.org:8080/#/c/17104/7/be/src/service/impala-server.cc@1114 PS7, Line 1114: if (FLAGS_dump_exec_request_path.empty()) : return; > nit: this can go on one line (and if it couldn't, Impala always uses braces Done http://gerrit.cloudera.org:8080/#/c/17104/7/be/src/service/impala-server.cc@1118 PS7, Line 1118: FLAGS_dump_exec_request_path + "/TExecRequest-" + dumpType + "." + : std::to_string(queryID.hi) + "-" + std::to_string(queryID.lo) > For constructing the filename, I think it would be cleaner to use the Subst Done http://gerrit.cloudera.org:8080/#/c/17104/7/be/src/service/impala-server.cc@1140 PS7, Line 1140: exec_request > It might be clearer for this to be 'external_exec_request' to emphasize tha Done http://gerrit.cloudera.org:8080/#/c/17104/7/be/src/service/impala-server.cc@1191 PS7, Line 1191: exec_request > Nit: From a style point, we prefer explicit checks against nullptr ("exec_r Done http://gerrit.cloudera.org:8080/#/c/17104/7/be/src/service/impala-server.cc@1198 PS7, Line 1198: exec_request > Nit: Same here (use explicit nullptr checks) Done http://gerrit.cloudera.org:8080/#/c/17104/7/be/src/service/impala-server.cc@1202 PS7, Line 1202: // Update the externally provided exec_request with Impala's query_id() : (*query_handle)->SetExecRequestQueryId(query_id); : // Update coordinator related internal addresses : (*query_handle)->SetExecRequestHostname( : ExecEnv::GetInstance()->configured_backend_address().hostname); : (*query_handle)->SetExecRequestKrpcAddress(ExecEnv::GetInstance()->krpc_address()); : // Update the field of 'local_time_zone' of 'query_ctx'. : (*query_handle)->SetExecRequestLocalTimeZone(query_ctx.local_time_zone); > might be nice to move this into QueryDriver::SetExternalPlan() Done http://gerrit.cloudera.org:8080/#/c/17104/7/be/src/service/impala-server.cc@1213 PS7, Line 1213: RETURN_IF_ERROR((*query_handle)->UpdateQueryStatus(exec_status)); > What's the point of this call? I think it will always be called with OK and Removed http://gerrit.cloudera.org:8080/#/c/17104/7/common/thrift/CMakeLists.txt File common/thrift/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/17104/7/common/thrift/CMakeLists.txt@70 PS7, Line 70: # Also do not generate ImpalaService.thrift because the generated code doesn't : # compile with hive if the thrift version in hive is 0.9.0 > Nit: We can remove this part of the comment Done http://gerrit.cloudera.org:8080/#/c/17104/7/fe/pom.xml File fe/pom.xml: http://gerrit.cloudera.org:8080/#/c/17104/7/fe/pom.xml@434 PS7, Line 434: <groupId>org.apache.hive</groupId> : <artifactId>hive-classification</artifactId> : <version>${hive.version}</version> > Does this bring in any dependencies that we need to exclude? I checked both and only hive-classification.jar was added -- To view, visit http://gerrit.cloudera.org:8080/17104 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iace716dd67290f08441857dc02d2428b0e335eaa Gerrit-Change-Number: 17104 Gerrit-PatchSet: 7 Gerrit-Owner: Kurt Deschler <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: John Sherman <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Tue, 02 Mar 2021 15:34:12 +0000 Gerrit-HasComments: Yes
