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 Fri, 29 Sep 2017 22:16:41 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 3:

(10 comments)

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@454
PS2, Line 454:  
> My understanding is that the copied bytes would also be freed via ReleasePr
I'm not fully familiar with how it works internally, but per my reading of the JVM code, all
this method does is to unlock the GC thread[1]. It looks to me like it doesn't free the copied
bytes (I could be wrong).

(If you think this case is possible, I think we should have code to perform cleanup of copies.
Please ignore if you think that is not possible.)

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


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

http://gerrit.cloudera.org:8080/#/c/8150/3/be/src/service/fe-support.cc@452
PS3, Line 452: the
remove


http://gerrit.cloudera.org:8080/#/c/8150/3/be/src/service/fe-support.cc@458
PS3, Line 458: Note that it does not matter if 'src' was copied
Why don't we treat it as an error? This could mean an OOM on the JVM?


http://gerrit.cloudera.org:8080/#/c/8150/3/be/src/service/fe-support.cc@471
PS3, Line 471: "
May be better to add that the JVM could've run out of memory (supportability).


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@289
PS2, Line 289:  - Storing the uncompressed data size allows decompression to pre-allocate
its output
             :    * - Decompression requires the exact compressed data size, so storing it
allows us to
             :    *   avoid compacting the returned byte[]
             :    */
> I don't think that's right because NativeLz4CompressBound() returns 0 if th
Oh, I thought this was an overflow check. Just realized LZ4 has an inbuilt limit of 2113929216
bytes(~2GB)


http://gerrit.cloudera.org:8080/#/c/8150/2/fe/src/main/java/org/apache/impala/service/FeSupport.java@306
PS2, Line 306: if (compressedSize < 0) {
             :       throw new InternalException(
> Not sure what you mean. A Java byte[] cannot be shrunk without copying it i
Ok, sorry for not being clear. My point is, why have two versions, Lz4CompressToByteBuffer()
and Lz4Compress(). Is that a requirement for the follow on patch?


http://gerrit.cloudera.org:8080/#/c/8150/3/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/3/fe/src/main/java/org/apache/impala/service/FeSupport.java@306
PS3, Line 306:     if (compressedSize < 0) {
Looks like this check should be <=.

Per the documentation of LZ4_compress(),

    return : the number of bytes written in buffer dest
             or 0 if the compression fails


http://gerrit.cloudera.org:8080/#/c/8150/3/fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java
File fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java:

http://gerrit.cloudera.org:8080/#/c/8150/3/fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java@66
PS3, Line 66:     } catch (Throwable e) {
I think fail() throws an "Error" (AssertionError) which this Throwable can catch. May be make
it Exception?


http://gerrit.cloudera.org:8080/#/c/8150/3/fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java@72
PS3, Line 72: e.printStackTrace();
unreachable?


http://gerrit.cloudera.org:8080/#/c/8150/3/fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java@78
PS3, Line 78:   private byte[] seqPayload(int numBytes) {
Add one liner comments on these helpers.



-- 
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: 3
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: Fri, 29 Sep 2017 22:16:41 +0000
Gerrit-HasComments: Yes

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