Skye Wanderman-Milne 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 Yup. This function never used the local ref so deleting it should have no functional effect, and maybe be slightly nicer to the JVM :) 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 = ... I'll change it here, if we wanna change it ever may as well be in this patch. This could arguably be turned into a new JniUtil method too but I didn't want to go too crazy with this patch. Line 464: &resultscanner_ > same comment here. especially confusing given that we could hit a RETURN_IF Done Line 548: RETURN_IF_ERROR(JniUtil::LocalToGlobalRef(env, cells_, &cells_)); > same Done 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 Done 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 referen Done 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 It's not, removed. Line 60: reinterpret_cast<jobject*> > nit: could get rid of this cast now that the template handles it. Done Line 68: RETURN_ERROR_IF_EXC(env); > let's not do it now, but I wonder if we should DeleteLocalRef the local_ref Local refs have the lifetime of the (native) function they're created in, so they're probably not sticking around for very long anyway. 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 Done. -- 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
