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 Sat, 09 Sep 2017 02:25:52 GMT
Bharath Vissapragada has posted comments on this change.

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


Patch Set 14:

(13 comments)

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

Line 103:       if (len <= 0) {
> I think it's better to remove and document the preconditions this function 
Removed.


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

Line 128:     if ((offset < 0) || (offset > b.length) || (len < 0)
> if (b == null) return;
Done


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

Line 42:           && (rand_.nextInt(10) < 8)) {
> Let's avoid non-determinism, otherwise a test failure may be hard to debug 
Fair point, switched to the counter logic.


Line 89:     while (testAllocator_.getAllocationFailures() < 10) {
> Not a big fan of the non-determinism. How about we run this loop a fixed (b
Now that the above logic is switched to a counter based one. this loop is deterministic as
well.


Line 91:       byte[] b = new byte[byteArraySize];
> Can we get away with allocating a single array of size byteArrayMaxSize?
Wanted to check varied allocation sizes, but other tests already do it, so  I'm moving to
a single size. I"m using 1MB rather than 1 GB.


Line 92:       writeAndCheck(b, 0, byteArraySize);
> How does this work? Is the NBAOS expected to be left in a good state after 
Yea this only runs because the write actually doesn't modify the underlying nboas, due to
the TestAllocator. Please correct me if I'm wrong, but isn't the unit test for checking that
we free in all edge cases? TNativeSerialzer test already writes multiple sized objects that
does the underlying allocate/free/copying?


Line 97:     writeAndCheck(b, -1, 10);
> Test that len == 0 works
Done


Line 100:     writeAndCheck(b, 5, 10);
> Test passing a null byte array
Done


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

Line 74:     UnsafeUtil.UNSAFE.freeMemory(result.buffer_ptr);
> put in finally block
Done


Line 86:    * buffer resizing capability in the underlying NativeByteArrayOutputStreami and
> typo: extraneous 'i' after NBAOS
Done


Line 107:     serializeTestObject(test15gb, protocolFactory);
> we also need to test the max 4GB
Added a test for ~3.5GB, made sure we serialize it ok, as we can't deserialize it on the Java
side. However it runs into occassional OOMs due to memory pressure. Still figuring out a way
to fix that. May be run testThriftLimits for only one protocol?


Line 119:     // The reason it is not a problem with Impala's catalog updates is because a
single
> I'd remove this sentence, it's not really true.
lol :) Done.


Line 139:     }
> Test that calling serialize() twice results in an exception.
Done


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