Joe McDonnell 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: (7 comments) Posting comments that I have so far 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' 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@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 Substitute() function that takes a template string and args: https://github.com/apache/impala/blob/master/be/src/gutil/strings/substitute.h#L172 There are several uses in this file to consult. 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 that this is ordinarily null. 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_request != nullptr") rather than implicit non-zero checks of pointer values. 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) 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 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? If you haven't already, it's worth checking mvn dependency:tree or diffing the fe/target/build-classpath.txt to see if this is adding 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 <[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: Mon, 01 Mar 2021 20:59:42 +0000 Gerrit-HasComments: Yes
