Bharath Vissapragada 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 > One downside is that some of the built-in usages of JNI in the JDK actually Just tried this locally. I see a bunch of warnings in the Catalog logs. You are talking about these right? WARNING in native method: JNI call made without checking exceptions when required to from CallLongMethodV WARNING in native method: JNI call made without checking exceptions when required to from CallLongMethodV Digging more into it it looks like these happen if "ExceptionOccurred()" is not called after calling "Call*Method()" and looking for this pattern in the code, we have multiple places that violate this. IMO these are legit warnings since not checking for exceptions can result in crashes in some edge cases (OOM/ JVM under heap pressure etc.) Fwiw, The problematic code that is causing this log spew is this method Status Catalog::GetCatalogVersion(long* version) { JNIEnv* jni_env = getJNIEnv(); JniLocalFrame jni_frame; RETURN_IF_ERROR(jni_frame.push(jni_env)); *version = jni_env->CallLongMethod(catalog_, get_catalog_version_id_); return Status::OK(); } That said, I agree that it is out of this fix's scope to fix all those problems. But IMO we should fix these warnings in a single patch and enable this flag along with it. 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()) { > FWIW, Joe tried to separate us from libhdfs's stuff but ran into bugs. http I was hoping we could add some checks while we are here and then fix the remaining ones later, but I agree to fix them all in a single go by getting rid of getJNIEnv() is cleaner. http://gerrit.cloudera.org:8080/#/c/11181/2/be/src/util/jni-util.h@273 PS2, Line 273: ObjectToResult > Should we add WARN_UNUSED_RESULT on the Status type itself? If the general I think we use this in most places Status is involved. There is a TODO about this in the Status class https://github.com/apache/impala/blob/master/be/src/common/status.h#L328 -- 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 <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 14 Aug 2018 06:19:04 +0000 Gerrit-HasComments: Yes
