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 <bhara...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
