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 <tw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to