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 <[email protected]> Gerrit-Reviewer: Aman Sinha <[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 19:50:17 +0000 Gerrit-HasComments: Yes
