You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tianyi Wang (Code Review)" <ge...@cloudera.org> on 2017/10/19 18:00:31 UTC
[Impala-ASF-CR] IMPALA-6060: Check the return value of JNI exception handling functions
Tianyi Wang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8334
Change subject: IMPALA-6060: Check the return value of JNI exception handling functions
......................................................................
IMPALA-6060: Check the return value of JNI exception handling functions
When JVM runs out of memory and throws an error to JNI, the error
handling code uses JNI to get the exception message, resulting in a
null pointer and crashing the process. This patch adds error handling
code to JniUtil::GetJniExceptionMsg().
Change-Id: Ie3ed88bf8739c56a066f2402727c8204e96aa116
---
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M fe/src/main/java/org/apache/impala/common/JniUtil.java
4 files changed, 32 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/8334/1
--
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: newchange
Gerrit-Change-Id: Ie3ed88bf8739c56a066f2402727c8204e96aa116
Gerrit-Change-Number: 8334
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6060: Check the return value of JNI exception handling functions
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8334/3/be/src/util/jni-util.cc
File be/src/util/jni-util.cc:
http://gerrit.cloudera.org:8080/#/c/8334/3/be/src/util/jni-util.cc@190
PS3, Line 190: auto msg = static_cast<jstring>(env->CallStaticObjectMethod(jni_util_class(),
> Same here and in other cases below: The use of auto obscures the code. This
It is somewhat obvious from the static_cast, of course. Still, auto generally adds cognitive burden to the reader unless it really helps readability
--
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: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Oct 2017 00:42:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6060: Check the return value of JNI exception handling functions
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8334/1/be/src/util/jni-util.h
File be/src/util/jni-util.h:
http://gerrit.cloudera.org:8080/#/c/8334/1/be/src/util/jni-util.h@159
PS1, Line 159: /// lives as long as this guard. jstr should not be null.
Usually such a comment indicates a precondition, but in this case a nullptr is handled gracefully by returning a status.
I think it's actually better to make it a precondition and use a DCHECK instead of returning a Status.
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@30
PS1, Line 30: Status JniUtfCharGuard::create(JNIEnv *env, jstring jstr, JniUtfCharGuard *out) {
fix pointer style while you are here: JNIEnv* env, etc.
http://gerrit.cloudera.org:8080/#/c/8334/1/be/src/util/jni-util.cc@197
PS1, Line 197: if (msg == nullptr) {
I think the real cause of the bug is here. The general rule is that you always need to check for exceptions after a JNI function call. I think we are missing a RETURN_ERROR_IF_EXC(env); at this point.
This handles checked as well as unchecked Java exceptions, including OOM.
http://gerrit.cloudera.org:8080/#/c/8334/1/be/src/util/jni-util.cc@208
PS1, Line 208: if (stack == nullptr) {
same here, I think the right fix is to add a RETURN_ERROR_IF_EXC(env);
--
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 <al...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Oct 2017 04:41:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6060: Check the return value of JNI exception handling functions
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Oct 2017 18:28:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6060: Check the return value of JNI exception handling functions
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1373/
--
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: 4
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Oct 2017 17:25:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6060: Check the return value of JNI exception handling functions
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8334
to look at the new patch set (#2).
Change subject: IMPALA-6060: Check the return value of JNI exception handling functions
......................................................................
IMPALA-6060: Check the return value of JNI exception handling functions
When JVM runs out of memory and throws an error to JNI, the error
handling code uses JNI to get the exception message, resulting in a
null pointer and crashing the process. This patch adds error handling
code to JniUtil::GetJniExceptionMsg().
Change-Id: Ie3ed88bf8739c56a066f2402727c8204e96aa116
---
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M fe/src/main/java/org/apache/impala/common/JniUtil.java
4 files changed, 28 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/8334/2
--
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: newpatchset
Gerrit-Change-Id: Ie3ed88bf8739c56a066f2402727c8204e96aa116
Gerrit-Change-Number: 8334
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6060: Check the return value of JNI exception handling functions
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang 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 3:
(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: env->ExceptionClear();
> This case of auto obscures the code. Might not be clear to everybody that t
Done
http://gerrit.cloudera.org:8080/#/c/8334/1/be/src/util/jni-util.cc@197
PS1, Line 197: }
> You are right that RETURN_ERROR_IF_EXC() does not make sense here.
Done
--
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: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Oct 2017 20:56:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6060: Check the return value of JNI exception handling functions
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang 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:
(1 comment)
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@197
PS1, Line 197: if (msg == nullptr) {
> I think the real cause of the bug is here. The general rule is that you alw
RETURN_ERROR_IF_EXC calls GetJniExceptionMsg.
--
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 <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Oct 2017 18:16:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6060: Check the return value of JNI exception handling functions
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8334 )
Change subject: IMPALA-6060: Check the return value of JNI exception handling functions
......................................................................
IMPALA-6060: Check the return value of JNI exception handling functions
When JVM runs out of memory and throws an error to JNI, the error
handling code uses JNI to get the exception message, resulting in a
null pointer and crashing the process. This patch adds error handling
code to JniUtil::GetJniExceptionMsg().
Change-Id: Ie3ed88bf8739c56a066f2402727c8204e96aa116
Reviewed-on: http://gerrit.cloudera.org:8080/8334
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M fe/src/main/java/org/apache/impala/common/JniUtil.java
4 files changed, 30 insertions(+), 18 deletions(-)
Approvals:
Alex Behm: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: Ie3ed88bf8739c56a066f2402727c8204e96aa116
Gerrit-Change-Number: 8334
Gerrit-PatchSet: 5
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6060: Check the return value of JNI exception handling functions
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang 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 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8334/1/be/src/util/jni-util.h
File be/src/util/jni-util.h:
http://gerrit.cloudera.org:8080/#/c/8334/1/be/src/util/jni-util.h@159
PS1, Line 159: /// lives as long as this guard. jstr should not be null.
> Usually such a comment indicates a precondition, but in this case a nullptr
Done
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@30
PS1, Line 30: Status JniUtfCharGuard::create(JNIEnv* env, jstring jstr, JniUtfCharGuard* out) {
> fix pointer style while you are here: JNIEnv* env, etc.
Done
http://gerrit.cloudera.org:8080/#/c/8334/1/be/src/util/jni-util.cc@197
PS1, Line 197: }
> RETURN_ERROR_IF_EXC calls GetJniExceptionMsg.
RETURN_ERROR_IF_EXC will call GetJniExceptionMsg again, which will result in an infinite recursion. And we may want to do some logging here since it is critical.
http://gerrit.cloudera.org:8080/#/c/8334/1/be/src/util/jni-util.cc@208
PS1, Line 208: }
> same here, I think the right fix is to add a RETURN_ERROR_IF_EXC(env);
See above.
--
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: 2
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Oct 2017 17:46:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6060: Check the return value of JNI exception handling functions
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 4: Code-Review+2
--
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: 4
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Oct 2017 17:24:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6060: Check the return value of JNI exception handling functions
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8334
to look at the new patch set (#3).
Change subject: IMPALA-6060: Check the return value of JNI exception handling functions
......................................................................
IMPALA-6060: Check the return value of JNI exception handling functions
When JVM runs out of memory and throws an error to JNI, the error
handling code uses JNI to get the exception message, resulting in a
null pointer and crashing the process. This patch adds error handling
code to JniUtil::GetJniExceptionMsg().
Change-Id: Ie3ed88bf8739c56a066f2402727c8204e96aa116
---
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M fe/src/main/java/org/apache/impala/common/JniUtil.java
4 files changed, 28 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/8334/3
--
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: newpatchset
Gerrit-Change-Id: Ie3ed88bf8739c56a066f2402727c8204e96aa116
Gerrit-Change-Number: 8334
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6060: Check the return value of JNI exception handling functions
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang 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 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8334/3/be/src/util/jni-util.cc
File be/src/util/jni-util.cc:
http://gerrit.cloudera.org:8080/#/c/8334/3/be/src/util/jni-util.cc@190
PS3, Line 190: jstring msg = static_cast<jstring>(env->CallStaticObjectMethod(jni_util_class(),
> It is somewhat obvious from the static_cast, of course. Still, auto general
Done
http://gerrit.cloudera.org:8080/#/c/8334/3/be/src/util/jni-util.cc@194
PS3, Line 194: string oom_msg = Substitute(oom_msg_template, "throwableToString");
> no auto please
Done
http://gerrit.cloudera.org:8080/#/c/8334/3/be/src/util/jni-util.cc@201
PS3, Line 201: jstring stack = static_cast<jstring>(env->CallStaticObjectMethod(jni_util_class(),
> no auto please
Done
http://gerrit.cloudera.org:8080/#/c/8334/3/be/src/util/jni-util.cc@205
PS3, Line 205: string oom_msg = Substitute(oom_msg_template, "throwableToStackTrace");
> no auto please
Done
--
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: 4
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Oct 2017 17:14:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6060: Check the return value of JNI exception handling functions
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 4: Verified+1
--
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: 4
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Oct 2017 21:22:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6060: Check the return value of JNI exception handling functions
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8334/3/be/src/util/jni-util.cc
File be/src/util/jni-util.cc:
http://gerrit.cloudera.org:8080/#/c/8334/3/be/src/util/jni-util.cc@190
PS3, Line 190: auto msg = static_cast<jstring>(env->CallStaticObjectMethod(jni_util_class(),
Same here and in other cases below: The use of auto obscures the code. This is a jstring which really is not obvious and does matter.
http://gerrit.cloudera.org:8080/#/c/8334/3/be/src/util/jni-util.cc@194
PS3, Line 194: auto oom_msg = Substitute(oom_msg_template, "throwableToString");
no auto please
http://gerrit.cloudera.org:8080/#/c/8334/3/be/src/util/jni-util.cc@201
PS3, Line 201: auto stack = static_cast<jstring>(env->CallStaticObjectMethod(jni_util_class(),
no auto please
http://gerrit.cloudera.org:8080/#/c/8334/3/be/src/util/jni-util.cc@205
PS3, Line 205: auto oom_msg = Substitute(oom_msg_template, "throwableToStackTrace");
no auto please
--
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: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Oct 2017 00:41:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6060: Check the return value of JNI exception handling functions
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8334
to look at the new patch set (#4).
Change subject: IMPALA-6060: Check the return value of JNI exception handling functions
......................................................................
IMPALA-6060: Check the return value of JNI exception handling functions
When JVM runs out of memory and throws an error to JNI, the error
handling code uses JNI to get the exception message, resulting in a
null pointer and crashing the process. This patch adds error handling
code to JniUtil::GetJniExceptionMsg().
Change-Id: Ie3ed88bf8739c56a066f2402727c8204e96aa116
---
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M fe/src/main/java/org/apache/impala/common/JniUtil.java
4 files changed, 30 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/8334/4
--
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: newpatchset
Gerrit-Change-Id: Ie3ed88bf8739c56a066f2402727c8204e96aa116
Gerrit-Change-Number: 8334
Gerrit-PatchSet: 4
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>