Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12660 )
Change subject: IMPALA-8250: Clean up JNI warnings. ...................................................................... Patch Set 1: (3 comments) > (3 comments) > > I'm seeing a lot of places in the HBase scanner where we call > GetByteArrayRegion or GetObjectArrayElement without checking for > exceptions (these can both throw ArrayIndexOutOfBoundsExceptions). > I'm not sure why -Xcheck:jni wouldn't detect these, but perhaps be > should do a scrub of the rest of the Impala code to make sure we > are checking for exceptions after each array call (there are > several other array-based JNI ops that throw ArrayIndexOutOfBoundsException > - > https://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/functions.html). > We can do that in a different patch though since it might be a > larger change. Yeah: this is surprising. I read the jniCheck.cpp code and it should be handling the GetObjectArray... methods. The most likely scenario is that our tests don't have coverage here. Or jniCheck isn't doing its business. I'm going to sprinkle in some of these where it's obvious. I've kicked off a suite of our tests with code coverage on. I'm also going to kick off a set of tests with the JNI checking and 'exhaustive' tests. Maybe there's coverage, but we're not running the queries against HBase. This discussion (from jniCheck.cpp) is also interesting. I didn't see the code that didn't warn about ArrayIndexOutOfBounds, but maybe that's what's happening. /** * Check whether or not a programmer has actually checked for exceptions. According * to the JNI Specification ("jni/spec/design.html#java_exceptions"): * * There are two cases where the programmer needs to check for exceptions without * being able to first check an error code: * * - The JNI functions that invoke a Java method return the result of the Java method. * The programmer must call ExceptionOccurred() to check for possible exceptions * that occurred during the execution of the Java method. * * - Some of the JNI array access functions do not return an error code, but may * throw an ArrayIndexOutOfBoundsException or ArrayStoreException. * * In all other cases, a non-error return value guarantees that no exceptions have been thrown. * * Programmers often defend against ArrayIndexOutOfBoundsException, so warning * for these functions would be pedantic. */ http://gerrit.cloudera.org:8080/#/c/12660/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12660/1//COMMIT_MSG@9 PS1, Line 9: Using HDFS_OPTS+="-Xcheck:jni" revealed a handful of warnings related to > It's LIBHDFS_OPTS right? Done http://gerrit.cloudera.org:8080/#/c/12660/1/be/src/runtime/hbase-table-factory.cc File be/src/runtime/hbase-table-factory.cc: http://gerrit.cloudera.org:8080/#/c/12660/1/be/src/runtime/hbase-table-factory.cc@97 PS1, Line 97: if (!s.ok()) LOG(INFO) << "Exception when cleaning up HBase" << s; > nit: space after HBase Done http://gerrit.cloudera.org:8080/#/c/12660/1/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/12660/1/be/src/service/frontend.cc@116 PS1, Line 116: ABORT_IF_ERROR(JniUtil::LoadJniMethod(jni_env, fe_class_, &(methods[i]))); > Might be understanding the code wrong, but shouldn't these be global refs s Based on https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#accessing_fields_and_methods , https://stackoverflow.com/questions/2093112/why-i-should-not-reuse-a-jclass-and-or-jmethodid-in-jni and http://www.latkin.org/blog/2016/02/01/jni-object-lifetimes-quick-reference/ (very nice!) and https://developer.android.com/training/articles/perf-jni.html#local_and_global_references say that jmethodid (which is the type here) doesn't need global references, but is invalidated if the class gets unloaded. Fortunately, the class is kept alive because fe_ stores it. I thought fe_class_ had the problem you suggest (it's a local reference), but it turns out that it's not used outside this method. I removed it from the class to make that obvious. -- To view, visit http://gerrit.cloudera.org:8080/12660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Gerrit-Change-Number: 12660 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 05 Mar 2019 20:34:15 +0000 Gerrit-HasComments: Yes
