You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Joe McDonnell (Code Review)" <ge...@cloudera.org> on 2021/03/01 20:59:42 UTC
[Impala-ASF-CR] IMPALA-10535: Add interface to ImpalaServer for execution of externally compiled statements
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 <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Mar 2021 20:59:42 +0000
Gerrit-HasComments: Yes