Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8334 )
Change subject: IMPALA-6060: Check the return value of JNI exception handling functions ...................................................................... Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/8334/1/be/src/util/jni-util.cc File be/src/util/jni-util.cc: http://gerrit.cloudera.org:8080/#/c/8334/1/be/src/util/jni-util.cc@193 PS1, Line 193: auto oom_msg_template = "$0 throws an unchecked throwable. JVM likely runs out of " This case of auto obscures the code. Might not be clear to everybody that this is a const char* and not a std::string. How about this err message: "$0 threw an unchecked exception. The JVM is likely out of memory (OOM)." http://gerrit.cloudera.org:8080/#/c/8334/1/be/src/util/jni-util.cc@197 PS1, Line 197: if (msg == nullptr) { > RETURN_ERROR_IF_EXC will call GetJniExceptionMsg again, which will result i You are right that RETURN_ERROR_IF_EXC() does not make sense here. I stand by my main point. We need to check for exceptions after every JNI function call, that's the standard practice and we should simply follow it here. Yes, checking for msg == nullptr might lead to the same result in practice, but that check is unnecessarily indirect. The right way to handle this case is: if (env->ExceptionOccurred()) { env->ExceptionClear(); ... } -- To view, visit http://gerrit.cloudera.org:8080/8334 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3ed88bf8739c56a066f2402727c8204e96aa116 Gerrit-Change-Number: 8334 Gerrit-PatchSet: 1 Gerrit-Owner: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Comment-Date: Mon, 23 Oct 2017 18:28:01 +0000 Gerrit-HasComments: Yes
