Alex Behm has posted comments on this change. Change subject: IMPALA-5881: Use native allocation while building catalog updates ......................................................................
Patch Set 16: (14 comments) http://gerrit.cloudera.org:8080/#/c/7955/16/be/src/catalog/catalog.cc File be/src/catalog/catalog.cc: Line 120: // Now that we deserialize the thrift objects into TGetAllCatalogObjectsResponse, deserialized (past tense) http://gerrit.cloudera.org:8080/#/c/7955/16/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: Line 84: // flag. remove this line http://gerrit.cloudera.org:8080/#/c/7955/16/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java: Line 61: " maximum limit of " + BUFFER_MAX_SIZE_LIMIT + " bytes"; remove first space in string Line 99: * - bufferPtr_>=0 use consistent spacing, i.e. bufferPtr_ >= 0 Line 102: Preconditions.checkState(bufferPtr_ >= 0); remove (documenting in comment is better here) http://gerrit.cloudera.org:8080/#/c/7955/16/fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java File fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java: Line 28: // Custom NativeAllocator that accounts the allocated and freed bytes and randomly the allocated/freed bytes and simulates allocation failures Line 39: // Throws an OOM on every alternate call to reallocate(). on every 2nd call Line 51: allocatedBytes_ = 0; single line Line 87: while (testAllocator_.getAllocationFailures() < 10) { I still don't think this is a great approach because it does not reflect real-world usage of the NBAOS. I think it's worth exhaustively testing all states (there are not that many). You could do something like this instead: // Test first allocation failure testAllocator_.setFailRealloc(true); try { Nbaos n = new Nbaos(testAllocator_); } catch () { // Fill in } // Test allocating failure from all possible states, allocating 64M at a time. int reallocs = 1; byte[] b = new byte[64 * 1024 * 1024 * 1024]; while (true) { TestAllocator alloc = new TestAllocator(); Nbaos n = new Nbaos(alloc); while (alloc.getReallocs() == reallocs) { n.write(b, 0, len); } alloc.setFailRealloc(true); writeAndCheck(); reallocs = alloc.getReallocs(); // If nbaos is at max capacity break out of loop } Line 91: writeAndCheck(b, 1, 0); These should be tested from a fresh Nbaos and not from the one that is already 'polluted' from previous tests. http://gerrit.cloudera.org:8080/#/c/7955/16/fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java File fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java: Line 58: // Deserialize the object at result.buffer_ptr and we confirm it is the same remove "we" Line 119: Assert.assertTrue(nativeSerializer.getSerializedBytesSize() >= 3584 * 1024 * 1024); Isn't there an assertGe() or something like that? Also print a message with the actual size for debugging Line 137: e.printStackTrace(); remove Line 166: testBasicSerialization(factory); Let's make these separate top-level tests, if's fine to repeat the loop over PROTOCOLS_TO_TEST. -- 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: 16 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
