impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Date Thu, 07 Sep 2017 19:00:03 GMT
Dimitris Tsirogiannis has posted comments on this change.

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


Patch Set 9:

(9 comments)

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


PS9, Line 94: reAllocateMemory
Unsafe.reallocateMemory()


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 them here? Am I missing
something?


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.


PS9, Line 78: huge
"huge" doesn't really convey much information here. Maybe a bit more explicit what we're testing
(>1GB, < 4GB)? Also, you may want to test multiple sizes to exercise all paths in the
reallocation logic (e.g. 500MB, 1GB, 1.5GB, 2GB). You can decide which ones make more sense.


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.


PS9, Line 99:   
nit: extra space


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 is larger than
4GB"


PS9, Line 105: never
ha, famous last words :)


-- 
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