impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <>
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:


Redo'ing the test a little, will update the new PS shortly.
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
File fe/src/main/java/org/apache/impala/service/

PS8, Line 51: import org.apache.impala.thrift.TNativeByteBuffer;
> dup (L49)
File fe/src/main/java/org/apache/impala/thrift/

PS9, Line 67: .
> nit: remove

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

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

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

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

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.
File fe/src/test/java/org/apache/impala/thrift/

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

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

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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Mostafa Mokhtar <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message