impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6060: Check the return value of JNI exception handling functions
Date Mon, 23 Oct 2017 18:28:01 GMT
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 <twang@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <twang@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Oct 2017 18:28:01 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message