Alex Behm has posted comments on this change. Change subject: IMPALA-5881: Use native allocation while building catalog updates ......................................................................
Patch Set 7: (15 comments) Still thinking about the warning/logging. http://gerrit.cloudera.org:8080/#/c/7955/1/be/src/catalog/catalog.cc File be/src/catalog/catalog.cc: PS1, Line 106: TNativeByteBuffer nbuffer; > makes sense. Done. Another possible issue is that we run out of memory when constructing the thrift objects, that would throw and be caught inside DeserializeThriftMsg() and turned into a Status. That should be a recoverable condition (hopefully we have enough mem next time). http://gerrit.cloudera.org:8080/#/c/7955/7/be/src/catalog/catalog.cc File be/src/catalog/catalog.cc: Line 114: uint32_t len = static_cast<uint32_t>(nbuffer.buffer_len); DCHECK that the buffer_len is smaller than max uint32_t to make it clear that the FE must have enforced this Line 119: JniUtil::CallJniMethod(catalog_, free_native_byte_buffer_id_, nbuffer); Let's check the status of this call and if non-ok emit LOG(ERROR) that n bytes have been leaked. http://gerrit.cloudera.org:8080/#/c/7955/7/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: Line 298: if (buffer.buffer_ptr <= 0) return; IIRC you said 0 should work. Convert to Preconditions check for < 0. http://gerrit.cloudera.org:8080/#/c/7955/7/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java: Line 26: * allocations happen on the native side and this class tracks the all allocations use native memory and this class tracks the corresponding pointer and size. Line 37: * BUFFER_RESIZE_INCREMENTS; end sentence with "." (not ";") Line 77: tryAllocate(initialSize, false); For simplicity, let's only allow the default initial size and not a custom one. Line 87: throw new IllegalArgumentException("Current size: " + bufferLen_ + Isn't this a potential memory leak? Who frees the native memory? Line 101: UnsafeUtil.UNSAFE.freeMemory(bufferPtr_); This doesn't make sense because bufferPtr_ is already 0. You need to use a temp variable like newBufferPtr. Line 103: "Failed to (re)allocateMemory() " + len + " bytes"); also print the value of reAllocate Line 108: * Write a byte array 'b' of length 'len at offset 'offset'. Resizes the buffer Not really accurate. Suggest: Writes 'len' bytes of 'b' starting at 'offset' into the stream.... Line 115: throw new IndexOutOfBoundsException( Isn't this a potential memory leak? Who frees the native memory? Line 122: newBufferSize += BUFFER_RESIZE_INCREMENTS; Infinite loop if overflow? Same question for L124. Maybe it's better to move the BUFFER_MAX_SIZE_LIMIT check in here. http://gerrit.cloudera.org:8080/#/c/7955/7/fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java File fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java: Line 62: * Currently that restriction is not enforced by this class and is the responsibility Seems easy to enforce this with a flag that gets checked at the beginning of this function and set in a finally block. http://gerrit.cloudera.org:8080/#/c/7955/7/fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java File fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java: Line 39: public class TNativeSerializerTest { It's so critical to get this right, I think it makes sense to hide the native allocations behind an interface so we can mock the allocator. For example, I'm thinking we would create a mock allocator that keeps track of the number of bytes allocated and freed. Then we force the allocator to return 0 or fail in various calls, so we can test that the freed is always equal to the allocated for all error/exception scenarios. I think we need to test all exceptions that can be thrown from the nbaos, and check that we called free correctly. -- 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: 7 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
