Alex Behm 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/14/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java: Line 110: if (newBufferPtr == 0) { > I think it is better to retain the check, just in case the JVM behavior cha Makes sense Line 128: if ((offset < 0) || (offset > b.length) || (len < 0) if (b == null) return; 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 (how to repro?). How about we just keep a long counter and fail whenever counter % 2 == 0 or something along those lines. Even using a fixed random seed seems unnecessarily complex. Line 89: while (testAllocator_.getAllocationFailures() < 10) { Not a big fan of the non-determinism. How about we run this loop a fixed (but large) number of times instead. Non-determinism will be hard to debug and give unpredictable test runtimes. Line 91: byte[] b = new byte[byteArraySize]; Can we get away with allocating a single array of size byteArrayMaxSize? Line 92: writeAndCheck(b, 0, byteArraySize); How does this work? Is the NBAOS expected to be left in a good state after write() throws an OOM? I think this test "quietly" assumes we are using the NativeTestAllocator, and it would not work with the real NativeAllocator (because we'd try to realloc a garbage pointer). Perhaps we should make the TestAllocator more real by actually allocating/freeing/copying Line 97: writeAndCheck(b, -1, 10); Test that len == 0 works Line 100: writeAndCheck(b, 5, 10); Test passing a null byte array 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 Line 86: * buffer resizing capability in the underlying NativeByteArrayOutputStreami and typo: extraneous 'i' after NBAOS Line 107: serializeTestObject(test15gb, protocolFactory); we also need to test the max 4GB 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. Line 139: } Test that calling serialize() twice results in an exception. -- 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
