Alex Behm 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: (8 comments) 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 Done 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? We only read the source, so we don't care if we get the bytes directly or a copy. We write into 'dst' and writing into a copy is not useful because then the caller has no data in the real 'dst'. Added comment and code to handle the is_copied case instead of returning an error. 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 (supportabilit Done http://gerrit.cloudera.org:8080/#/c/8150/3/be/src/service/fe-support.cc@472 PS3, Line 472: env->ReleasePrimitiveArrayCritical(src, src_data, 0); I think there was a subtle bug here with not calling ReleasePrimitiveArrayCritical() on the dst if it was copied. I restored the scoped array critical class. 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 <=. Done 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 Reworked. Still want to catch Throwable here to check for OOM. See test in L162. 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? Done 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. Done -- 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.b...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Comment-Date: Sat, 30 Sep 2017 04:03:47 +0000 Gerrit-HasComments: Yes
