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-5881: Use native allocation while building catalog updates
Date Thu, 07 Sep 2017 23:55:02 GMT
Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................


Patch Set 9:

(14 comments)

Redo'ing the test a little, will update the new PS shortly.

http://gerrit.cloudera.org:8080/#/c/7955/8/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:

PS8, Line 603: // Struct containing eight strings, used for testing.
> Mention what are we testing with this weird looking struct. Also, leave a T
Done


http://gerrit.cloudera.org:8080/#/c/7955/8/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:

PS8, Line 51: import org.apache.impala.thrift.TNativeByteBuffer;
> dup (L49)
Done


http://gerrit.cloudera.org:8080/#/c/7955/9/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:

PS9, Line 67: .
> nit: remove
Done


Line 87:   private NativeByteArrayOutputStream(long initialSize) {
> Merge into the public constructor to remove some code and never mention a c
Done


PS9, Line 94: reAllocateMemory
> Unsafe.reallocateMemory()
Done


Line 99:       Preconditions.checkState(bufferPtr_ >= 0);
> needs to go above the try, otherwise we'll try to free an invalid ptr in L1
Done


Line 104:         throw new RuntimeException(BUFFER_LIMIT_EXCEEDED_MSG + ": " + len);
> IllegalArgumentException?
Done


PS9, Line 159: public void writeTo(OutputStream out) {
             :     throw new IllegalStateException("Not implemented.");
             :   }
             : 
             :   public byte toByteArray()[] {
             :     throw new IllegalStateException("Not implemented.");
             :   }
             : 
             :   public int size() {
             :     throw new IllegalStateException("Not implemented.");
             :   }
> These are not defined in OutputStream, so what is the point of defining the
These are from ByteArrayOutputStream. Given we mimic it, someone might expect to call them.
I'm fine with keeping/removing them.


http://gerrit.cloudera.org:8080/#/c/7955/9/fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
File fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java:

PS9, Line 38: @SuppressWarnings("restriction")
> explain? Also add a brief description of what this class tests.
IDE magic, removed


PS9, Line 78: huge
> "huge" doesn't really convey much information here. Maybe a bit more explic
Done


PS9, Line 85: char[] chars = new char[128 * 1024 * 1024];
            :     Arrays.fill(chars, 'a');
            :     String hugeString = new String(chars);
> nit: see if you can use Guava's Strings.repeat() here, may be cheaper.
Done


PS9, Line 99:   
> nit: extra space
Some kind of visual effect, don't think there are two spaces.


PS9, Line 99: Create a huge string of size 512MB. The combined size of the  test object
            :     // crosses 4GB.
> Maybe say something like "create an object with a serialization size which 
Done


PS9, Line 105: never
> ha, famous last words :)
lol :D


-- 
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message