Tim Armstrong has posted comments on this change. Change subject: IMPALA-5327: Handle return of JNI GetStringUTFChar ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/7642/1/be/src/util/jni-util.cc File be/src/util/jni-util.cc: Line 183: LOG_AT_LEVEL(google::FATAL) << get_str_fail_message; LOG(FATAL) actually kills the process. How about LOG(ERROR) instead? PS1, Line 184: MEM_ALLOC_FAILED This error message requires a parameter (see the string in common/thrift/generate_error_codes.py). How about just returning Status(get_str_fail_message); Line 193: LOG_AT_LEVEL(google::FATAL) << get_str_fail_message; Same here. PS1, Line 202: prefix + msg_str) We usually prefer Substitute() for cases like this. E.g. return Status(Substitute("$0: $1", prefix, msg_str)); http://gerrit.cloudera.org:8080/#/c/7642/1/be/src/util/logging-support.cc File be/src/util/logging-support.cc: Line 48: auto get_str_fail_message = "GetStringUTFChars failed. Probable OOM on JVM side"; Same comments here. How about we add a function to the JNIUtil class that does this to avoid repeating the logic. E.g. Status GetStringUTFChars(....); -- To view, visit http://gerrit.cloudera.org:8080/7642 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I47eed045f21c6ed3a8decf422ce8429fc66c4322 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tianyi Wang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
