Dan Hecht has posted comments on this change. Change subject: IMPALA-3378/IMPALA-3379: fix various JNI issues ......................................................................
Patch Set 5: (10 comments) http://gerrit.cloudera.org:8080/#/c/2820/5/be/src/exec/external-data-source-executor.cc File be/src/exec/external-data-source-executor.cc: Line 63: &executor_class_)); this looks good, but does have the side-effect of DeleteLocalRef() where we didn't before. I think it's okay, but just want to mention it. http://gerrit.cloudera.org:8080/#/c/2820/5/be/src/exec/hbase-table-scanner.cc File be/src/exec/hbase-table-scanner.cc: Line 309: scan_ = env->NewObject(scan_cl_, scan_ctor_); this was preexisting, but how about: jobject local_scan = ... just because it's a little confusing to treat scan_ as both a local and then global ref, especially since we FreeGlobalRef it. okay to defer if you want to keep this patch minimal. Line 464: &resultscanner_ same comment here. especially confusing given that we could hit a RETURN_IF_ERROR path. Line 548: RETURN_IF_ERROR(JniUtil::LocalToGlobalRef(env, cells_, &cells_)); same http://gerrit.cloudera.org:8080/#/c/2820/5/be/src/exprs/hive-udf-call.cc File be/src/exprs/hive-udf-call.cc: Line 268: delete jni_ctx; was this a leak before? mention this fix in the commit message also http://gerrit.cloudera.org:8080/#/c/2820/5/be/src/exprs/hive-udf-call.h File be/src/exprs/hive-udf-call.h: Line 108: /// Init(). maybe say that they live for the entire process lifetime and so the references aren't deleted. http://gerrit.cloudera.org:8080/#/c/2820/5/be/src/util/jni-util.cc File be/src/util/jni-util.cc: Line 59: reinterpret_cast<jobject> is this cast needed? doesn't seem like it should be needed given _jclass is a subclass of _jobject. Line 60: reinterpret_cast<jobject*> nit: could get rid of this cast now that the template handles it. Line 68: RETURN_ERROR_IF_EXC(env); let's not do it now, but I wonder if we should DeleteLocalRef the local_ref to avoid accumulating too many local refs. http://gerrit.cloudera.org:8080/#/c/2820/5/be/src/util/jni-util.h File be/src/util/jni-util.h: Line 190: are empty I'm not sure them being empty is all that important, I think we should just delete that part. -- To view, visit http://gerrit.cloudera.org:8080/2820 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8cd089e355d2ee2d5ace81f05b214272c05cf941 Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-HasComments: Yes
