[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression

["Bharath Vissapragada (Code Review)" <ge...@cloudera.org>]

Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/8150 )

Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/compression
......................................................................


Patch Set 5: Code-Review+1

(4 comments)

The patch looks good to me once the comments are fixed.

http://gerrit.cloudera.org:8080/#/c/8150/5/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:

http://gerrit.cloudera.org:8080/#/c/8150/5/be/src/service/fe-support.cc@457
PS5, Line 457: // It does not matter if 'src' is copied because we only read from it.
nit: May be document src_off, dst_off and return value for readability.


http://gerrit.cloudera.org:8080/#/c/8150/5/be/src/service/fe-support.cc@481
PS5, Line 481:     env->SetByteArrayRegion(dst, 0, len, reinterpret_cast<jbyte*>(dst_data));
Check the return value of this? Given it made a copy, its likely that this call can fail too?


http://gerrit.cloudera.org:8080/#/c/8150/5/be/src/util/jni-util.h
File be/src/util/jni-util.h:

http://gerrit.cloudera.org:8080/#/c/8150/5/be/src/util/jni-util.h@184
PS5, Line 184: !is_copy_ &&
My understanding is that we should call ReleasePrimitiveArrayCritical() irrespective of whether a copy is made. My reasoning is that it seems to be unblocking the GC thread, which I think is required.


http://gerrit.cloudera.org:8080/#/c/8150/5/be/src/util/jni-util.h@192
PS5, Line 192: ;
WARN_UNUSED_RESULT



-- 
To view, visit http://gerrit.cloudera.org:8080/8150
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I237802944875b07080db0159ff8ec548150fd95e
Gerrit-Change-Number: 8150
Gerrit-PatchSet: 5
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 02 Oct 2017 23:19:59 +0000
Gerrit-HasComments: Yes