impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Date Thu, 28 Sep 2017 20:51:44 GMT
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 2:

(8 comments)

Didn't see the test class, but got some comments on the logic.

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

http://gerrit.cloudera.org:8080/#/c/8150/2/be/src/service/fe-support.cc@450
PS2, Line 450: // whether to compress or decompress the 'src' into 'dst'.
May be worth commenting about GetPrimitiveArrayCritical() and lifecycle of the buffer. Looks
like it is up to the JVM to GC it eventually.


http://gerrit.cloudera.org:8080/#/c/8150/2/be/src/service/fe-support.cc@451
PS2, Line 451: extern "C"
Is extern required for this method? Also, no return type?


http://gerrit.cloudera.org:8080/#/c/8150/2/be/src/service/fe-support.cc@453
PS2, Line 453:     jbyteArray dst, jint dst_off, jint dst_len, jboolean compress) {
DCHECK_GE(dst_len, LZ4_compressBound(src_size))?


http://gerrit.cloudera.org:8080/#/c/8150/2/be/src/service/fe-support.cc@454
PS2, Line 454: 0
nit: Not sure if this was intentional, but you are passing a nullptr argument. JDK can handle
it ok [1], but looks like you need to pass a pointer to 0/1. 

Also, we should probably make sure the (isCopy==false) (after the call), incase the JVM does
a copy for some reason, we should free the copied native bytes.

[1] http://hg.openjdk.java.net/jdk8/jdk8/hotspot/file/87ee5ee27509/src/share/vm/prims/jni.cpp#l4244


http://gerrit.cloudera.org:8080/#/c/8150/2/be/src/service/fe-support.cc@455
PS2, Line 455:   if (src_data == nullptr) return -1;
Do we still need to call ReleasePrimitiveArrayCritical() in this case? Also, I see that you
implemented a ScopedArrayCritical class to do that (much cleaner). Wondering why you've undone
the changes? (same below)


http://gerrit.cloudera.org:8080/#/c/8150/2/fe/src/main/java/org/apache/impala/service/FeSupport.java
File fe/src/main/java/org/apache/impala/service/FeSupport.java:

http://gerrit.cloudera.org:8080/#/c/8150/2/fe/src/main/java/org/apache/impala/service/FeSupport.java@285
PS2, Line 285:    */
Probably worth commenting about the format of the output buffer.


http://gerrit.cloudera.org:8080/#/c/8150/2/fe/src/main/java/org/apache/impala/service/FeSupport.java@289
PS2, Line 289: if (dstLen <= 0) {
             :       throw new InternalException(
             :           "Payload is too big for LZ4 compression: " + payload.length);
             :     }
Move this below L294?


http://gerrit.cloudera.org:8080/#/c/8150/2/fe/src/main/java/org/apache/impala/service/FeSupport.java@306
PS2, Line 306: ByteBuffer.wrap(dst).putInt(0, payload.length);
             :     ByteBuffer.wrap(dst).putInt(4, compressedSize);
Can't we return the resized buffer directly here? Basically merge the logic of Lz4CompressToByteBuffer
into this method?



-- 
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: 2
Gerrit-Owner: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Sep 2017 20:51:44 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message