impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression
Date Mon, 02 Oct 2017 23:49:16 GMT
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 <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: Mon, 02 Oct 2017 23:49:16 +0000
Gerrit-HasComments: Yes

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