Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-5881: Use native allocation while building catalog updates ......................................................................
Patch Set 7: (13 comments) Addressed comments other than the test file. 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 th Done 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 by Done 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. Done 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 p Done Line 37: * BUFFER_RESIZE_INCREMENTS; > end sentence with "." (not ";") Done Line 77: tryAllocate(initialSize, false); > For simplicity, let's only allow the default initial size and not a custom Done Line 87: throw new IllegalArgumentException("Current size: " + bufferLen_ + > Isn't this a potential memory leak? Who frees the native memory? Redid the logic a little. Should cover this. Line 101: UnsafeUtil.UNSAFE.freeMemory(bufferPtr_); > This doesn't make sense because bufferPtr_ is already 0. You need to use a Ouch, good catch. Line 103: "Failed to (re)allocateMemory() " + len + " bytes"); > also print the value of reAllocate Done Line 108: * Write a byte array 'b' of length 'len at offset 'offset'. Resizes the buffer > Not really accurate. Suggest: Done Line 115: throw new IndexOutOfBoundsException( > Isn't this a potential memory leak? Who frees the native memory? Done Line 122: newBufferSize += BUFFER_RESIZE_INCREMENTS; > Infinite loop if overflow? Same question for L124. Done 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 o 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: 7 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
