Thomas Tauber-Marshall 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: (6 comments) 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) 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 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 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 around blocks) Some of these formatting issues can be avoided if you check out clang-format-diff, see: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536 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() 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 not do anything -- 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 <kdesc...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: John Sherman <j...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Mon, 01 Mar 2021 23:56:42 +0000 Gerrit-HasComments: Yes
