Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11181 )
Change subject: IMPALA-7421. Static methods use wrong JNI call function ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/11181/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11181/2//COMMIT_MSG@12 PS2, Line 12: -Xcheck:jni > Wondering if we should enable it in the DEBUG build by default. One downside is that some of the built-in usages of JNI in the JDK actually spew some warnings with -Xcheck:jni. I'd be nervous that if we turn it on in debug mode, people might get confused by these warnings. What do you think? http://gerrit.cloudera.org:8080/#/c/11181/2/be/src/util/jni-util.h File be/src/util/jni-util.h: http://gerrit.cloudera.org:8080/#/c/11181/2/be/src/util/jni-util.h@250 PS2, Line 250: env_(getJNIEnv()) { > I recently saw an Impalad crash because getJNIEnv() returned a nullptr and Should we fix that separately? It seems like there are 39 calls to getJNIEnv() in the code and none of them have assertions. It also seems like getJNIEnv() itself is coming from libhdfs and does a lot of HDFS-specific stuff, perhaps needlessly complex for what we need and we could do our own simpler implementation with the appropriate CHECKs built in? http://gerrit.cloudera.org:8080/#/c/11181/2/be/src/util/jni-util.h@273 PS2, Line 273: ObjectToResult > WARN_UNUSED_RESULT for these... Should we add WARN_UNUSED_RESULT on the Status type itself? If the general style is that all functions that return Status should be annotated as such, maybe it would be easier to just put the annotation on the type instead of the function. Or does that not work in gcc? (I know it works in clang) -- To view, visit http://gerrit.cloudera.org:8080/11181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If7cde6ca91613b63afe5307f4d819fb24cb17fd6 Gerrit-Change-Number: 11181 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Mon, 13 Aug 2018 16:21:55 +0000 Gerrit-HasComments: Yes
