[Impala-ASF-CR] IMPALA-10522: Support external use of frontend libraries

["Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>]

Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/17115 )

Change subject: IMPALA-10522: Support external use of frontend libraries
......................................................................


Patch Set 8:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/17115/8/be/src/runtime/exec-env.h
File be/src/runtime/exec-env.h:

http://gerrit.cloudera.org:8080/#/c/17115/8/be/src/runtime/exec-env.h@82
PS8, Line 82: class ExecEnv {
Might be nice to flesh out the class comment here a little about what it means to have an ExecEnv that's for an external FE, eg. "Not everything is going to be initialized, just at least the stuff needed to do the FeSupport functions. TODO: separate this out better"


http://gerrit.cloudera.org:8080/#/c/17115/8/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

http://gerrit.cloudera.org:8080/#/c/17115/8/be/src/runtime/exec-env.cc@253
PS8, Line 253:     frontend_(external_fe ? nullptr : new Frontend()),
Its a bit tricky to trace through and prove that the variables here that aren't getting initialized anymore (frontend_ and default_fs_ below) aren't actually used in any of the FeSupport functionality.

You could store a 'external_fe_' variable and DCHECK on it when those things are accessed, for a little extra safety.


http://gerrit.cloudera.org:8080/#/c/17115/8/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:

http://gerrit.cloudera.org:8080/#/c/17115/8/be/src/service/fe-support.cc@83
PS8, Line 83: InitForFeTests
This function name is unfortunate now. Not sure what a better options is - InitForFeSupport() maybe?


http://gerrit.cloudera.org:8080/#/c/17115/8/fe/src/main/java/org/apache/impala/service/FeSupport.java
File fe/src/main/java/org/apache/impala/service/FeSupport.java:

http://gerrit.cloudera.org:8080/#/c/17115/8/fe/src/main/java/org/apache/impala/service/FeSupport.java@489
PS8, Line 489:   public static void loadLibrary() {
I'm a little confused by this function and the 'externalFE_' variable - I guess the expected usage is that the external FE could call "loadLibrary(true)" first, and then subsequent calls to "loadLibrary()", eg. in functions in this file like CacheJar(), LookupSymbol(), etc. will get the "externalFE" version of this function (though that won't actually matter, as 'loaded_' would have to be true in those cases)?

Maybe it would be clearer to have a "setExternalFE()" function that the external FE can call, which sets 'externalFE_' and which is required to be called before any calls to loadLibrary(). You could presumably check that 'loaded_' is false in setExternalFE() for a little extra safety.



-- 
To view, visit http://gerrit.cloudera.org:8080/17115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e3a84721ba196ec00773ce2923b19610b90edd9
Gerrit-Change-Number: 17115
Gerrit-PatchSet: 8
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@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 19:50:17 +0000
Gerrit-HasComments: Yes