You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Alex Behm (Code Review)" <ge...@cloudera.org> on 2017/09/27 05:17:30 UTC
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Alex Behm has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8150
Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/compression
......................................................................
IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Adds LZ4 de/compression in FeSupport geared towards
eventually compressing catalog objects end-to-end
in their journey from the catalogd over the statestored
to coordinator impalads.
A follow-on change will use the new LZ4 functions to
do the actual compression.
Testing:
- Added a new unit test
Change-Id: I237802944875b07080db0159ff8ec548150fd95e
---
M be/src/service/fe-support.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M fe/src/main/java/org/apache/impala/service/FeSupport.java
A fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java
5 files changed, 424 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/8150/1
--
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: newchange
Gerrit-Change-Id: I237802944875b07080db0159ff8ec548150fd95e
Gerrit-Change-Number: 8150
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Hello Philip Zeyliger,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8150
to look at the new patch set (#2).
Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/compression
......................................................................
IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Adds LZ4 de/compression in FeSupport geared towards
eventually compressing catalog objects end-to-end
in their journey from the catalogd over the statestored
to coordinator impalads.
A follow-on change will use the new LZ4 functions to
do the actual compression.
Testing:
- Added a new unit test
Change-Id: I237802944875b07080db0159ff8ec548150fd95e
---
M be/src/service/fe-support.cc
M fe/src/main/java/org/apache/impala/service/FeSupport.java
A fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java
3 files changed, 354 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/8150/2
--
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: newpatchset
Gerrit-Change-Id: I237802944875b07080db0159ff8ec548150fd95e
Gerrit-Change-Number: 8150
Gerrit-PatchSet: 2
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 2:
(1 comment)
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@452
PS2, Line 452: NativeLz4Internal(JNIEnv* env, jbyteArray src, jint src_off, jint src_len,
PhilZ, this is code that the library would handle for us. I don't think it's worth pulling in a separate library for these simple few lines. Also, the error handling in this function is better than in the library, imo. The library throws an OOM for the L455 and L458 cases here without calling ReleasePrimitiveArrayCritical() which seems bad.
--
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 <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Sep 2017 17:25:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Philip Zeyliger,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8150
to look at the new patch set (#3).
Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/compression
......................................................................
IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Adds LZ4 de/compression in FeSupport geared towards
eventually compressing catalog objects end-to-end
in their journey from the catalogd over the statestored
to coordinator impalads.
A follow-on change will use the new LZ4 functions to
do the actual compression.
Testing:
- Added a new unit test
Change-Id: I237802944875b07080db0159ff8ec548150fd95e
---
M be/src/service/fe-support.cc
M fe/src/main/java/org/apache/impala/service/FeSupport.java
A fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java
3 files changed, 374 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/8150/3
--
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: newpatchset
Gerrit-Change-Id: I237802944875b07080db0159ff8ec548150fd95e
Gerrit-Change-Number: 8150
Gerrit-PatchSet: 3
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>
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 <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: Sat, 30 Sep 2017 04:03:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 2:
(1 comment)
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@455
PS2, Line 455: if (src_data == nullptr) return -1;
> See documentation and example in:
Forgot to respond to your comment regarding ScopedArrayCritical.
I removed that code because the case here is so simple that we don't really need a ScopedArrayCritical.
I'm happy to add it back, but I feel we should prefer adding it once it really becomes necessary (i.e. we have some non-trivial cleanup cases).
--
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 <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: Fri, 29 Sep 2017 04:01:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "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 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 <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: Thu, 28 Sep 2017 20:51:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 2:
(2 comments)
Just realized I forgot these comments, sorry.
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: 0
> I'm not fully familiar with how it works internally, but per my reading of
Why would it need to free the copied bytes? The copied bytes are on the Java heap and will be GC'ed as usual (just like the original array once the GC is unlocked again)
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@306
PS2, Line 306: ByteBuffer.wrap(dst).putInt(0, payload.length);
: ByteBuffer.wrap(dst).putInt(4, compressedSize);
> Ok, sorry for not being clear. My point is, why have two versions, Lz4Compr
We may not need the ByteBuffer version. Removed.
--
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 <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: Sat, 30 Sep 2017 04:23:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Philip Zeyliger,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8150
to look at the new patch set (#4).
Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/compression
......................................................................
IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Adds LZ4 de/compression in FeSupport geared towards
eventually compressing catalog objects end-to-end
in their journey from the catalogd over the statestored
to coordinator impalads.
A follow-on change will use the new LZ4 functions to
do the actual compression.
Testing:
- Added a new unit test
Change-Id: I237802944875b07080db0159ff8ec548150fd95e
---
M be/src/service/fe-support.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M fe/src/main/java/org/apache/impala/service/FeSupport.java
A fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java
5 files changed, 426 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/8150/4
--
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: newpatchset
Gerrit-Change-Id: I237802944875b07080db0159ff8ec548150fd95e
Gerrit-Change-Number: 8150
Gerrit-PatchSet: 4
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>
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Philip Zeyliger,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8150
to look at the new patch set (#5).
Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/compression
......................................................................
IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Adds LZ4 de/compression in FeSupport geared towards
eventually compressing catalog objects end-to-end
in their journey from the catalogd over the statestored
to coordinator impalads.
A follow-on change will use the new LZ4 functions to
do the actual compression.
Testing:
- Added a new unit test
Change-Id: I237802944875b07080db0159ff8ec548150fd95e
---
M be/src/service/fe-support.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M fe/src/main/java/org/apache/impala/service/FeSupport.java
A fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java
5 files changed, 401 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/8150/5
--
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: newpatchset
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>
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 5:
(4 comments)
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.
Documented return values.
I think the 'off' and 'len' params are standard enough to not need extra comments.
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
Function returns void.
Added code to handle exceptions.
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() irr
Good catch, that was a bug. Fixed.
http://gerrit.cloudera.org:8080/#/c/8150/5/be/src/util/jni-util.h@192
PS5, Line 192: ;
> WARN_UNUSED_RESULT
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: 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:49:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has abandoned this change. ( http://gerrit.cloudera.org:8080/8150 )
Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/compression
......................................................................
Abandoned
Will rework to simplify.
--
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: abandon
Gerrit-Change-Id: I237802944875b07080db0159ff8ec548150fd95e
Gerrit-Change-Number: 8150
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger 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:
(1 comment)
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@452
PS2, Line 452: NativeLz4Internal(JNIEnv* env, jbyteArray src, jint src_off, jint src_len,
> PhilZ, this is code that the library would handle for us. I don't think it'
Sounds good to me. Thanks.
--
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 <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: Thu, 28 Sep 2017 22:12:45 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Philip Zeyliger,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8150
to look at the new patch set (#6).
Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/compression
......................................................................
IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Adds LZ4 de/compression in FeSupport geared towards
eventually compressing catalog objects end-to-end
in their journey from the catalogd over the statestored
to coordinator impalads.
A follow-on change will use the new LZ4 functions to
do the actual compression.
Testing:
- Added a new unit test
Change-Id: I237802944875b07080db0159ff8ec548150fd95e
---
M be/src/service/fe-support.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M fe/src/main/java/org/apache/impala/service/FeSupport.java
A fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java
5 files changed, 411 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/8150/6
--
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: newpatchset
Gerrit-Change-Id: I237802944875b07080db0159ff8ec548150fd95e
Gerrit-Change-Number: 8150
Gerrit-PatchSet: 6
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>
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "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
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has abandoned this change. ( http://gerrit.cloudera.org:8080/8150 )
Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/compression
......................................................................
Abandoned
Putting this on hold as it's not clear we will proceed with this route.
--
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: abandon
Gerrit-Change-Id: I237802944875b07080db0159ff8ec548150fd95e
Gerrit-Change-Number: 8150
Gerrit-PatchSet: 6
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>
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger 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 1:
It looks like libraries like https://github.com/lz4/lz4-java exist to do this sort of thing. What's our thinking on using them instead of implementing our own wrappers?
--
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: 1
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Sep 2017 16:20:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 6: Code-Review+1
Carry +1
--
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: 6
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:49:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 2:
(9 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@450
PS2, Line 450: // whether to compress or decompress the 'src' into 'dst'.
> May be worth commenting about GetPrimitiveArrayCritical() and lifecycle of
Done
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?
Done
http://gerrit.cloudera.org:8080/#/c/8150/2/be/src/service/fe-support.cc@452
PS2, Line 452: NativeLz4Internal(JNIEnv* env, jbyteArray src, jint src_off, jint src_len,
> Sounds good to me. Thanks.
Done
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))?
Why DCHECK? It's not a precondition of this function. The compression will fail gracefully.
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 argume
My understanding is that the copied bytes would also be freed via ReleasePrimitiveArrayCritical(), so I don't see the flow changing even if is_copy is true.
I added code to deal with is_copy and added comments to explain.
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
See documentation and example in:
http://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/functions.html
I added comments here.
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.
Done
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?
I don't think that's right because NativeLz4CompressBound() returns 0 if the payload is too big. If we do the check after adding 8 in L294, then the check won't work anymore.
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
Not sure what you mean. A Java byte[] cannot be shrunk without copying it into a new one.
The ByteBuffer trick works without copying. The purpose of the ByteBuffer version is to hand it to a thrift binary which will only copy as many bytes as the ByteBuffer has left (which might be different than the size of the byte[] backing the ByteBuffer).
We could switch to using a ByteBuffer everywhere but that seems kinda complicated.
--
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 <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: Fri, 29 Sep 2017 03:59:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 1:
I had looked at that library but concluded we would not want to pull it in as-is because it includes a lot of stuff we don't need and also the LZ4 code - but we want to use the LZ4 code from our toolchain. Reimplementing the JNI wrappers we would need seems so simple that it's not worth the trouble pulling the library and integrating it into our build/toolchain.
However, looking at the library code more carefully, I think we make the implementation I posted here much simpler, so let me try and rework that.
--
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: 1
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Sep 2017 05:36:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has restored this change. ( http://gerrit.cloudera.org:8080/8150 )
Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/compression
......................................................................
Restored
--
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: restore
Gerrit-Change-Id: I237802944875b07080db0159ff8ec548150fd95e
Gerrit-Change-Number: 8150
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Posted by "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 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 <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: Fri, 29 Sep 2017 22:16:41 +0000
Gerrit-HasComments: Yes