Alex Behm has posted comments on this change. Change subject: IMPALA-5881: Use native allocation while building catalog updates ......................................................................
Patch Set 13: (8 comments) Still looking at tests. http://gerrit.cloudera.org:8080/#/c/7955/13/be/src/catalog/catalog.cc File be/src/catalog/catalog.cc: Line 171: LOG(ERROR) << "Failed to free buffers. Leaked memory of size: " Failed to free buffer. (singular buffer) http://gerrit.cloudera.org:8080/#/c/7955/13/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: Line 155: FileUtils.byteCountToDisplaySize( We have a PrintUtils.printBytes(), use that for consistency. Maybe we should eventually remove that in favor of FileUtils.byteCountToDisplaySize() but not in this patch. http://gerrit.cloudera.org:8080/#/c/7955/13/fe/src/main/java/org/apache/impala/thrift/NativeAllocator.java File fe/src/main/java/org/apache/impala/thrift/NativeAllocator.java: Line 23: * methods. Mention that we have this wrapper so we can override the methods for a testing. Line 29: public long allocate(long bufferPtr, long size) { reallocate() 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 92: public NativeByteArrayOutputStream() { this(new NativeAllocator()); } extra space after public Line 103: if (len <= 0) { Is this dead code? Not sure we can ever get here (how would we write a test to cover this code path?) PS13, Line 100: private void tryAllocate(long len) { : Preconditions.checkState(bufferPtr_ >= 0); : try { : if (len <= 0) { : throw new IllegalArgumentException(INVALID_BUFFER_LEN_MSG + ": " + len); : } : if (len >= BUFFER_MAX_SIZE_LIMIT) { : throw new IllegalArgumentException(BUFFER_LIMIT_EXCEEDED_MSG + ": " + len); : } : long newBufferPtr = nativeAllocator_.allocate(bufferPtr_, len); : if (newBufferPtr == 0) { : throw new IllegalStateException(ALLOCATION_FAILED_MSG + ": " + len : + " Is realloc: " + (bufferPtr_ > 0)); : } : bufferPtr_ = newBufferPtr; : bufferLen_ = len; : } catch (Throwable e) { : nativeAllocator_.free(bufferPtr_); : throw e; : } : } > Wanted to keep the NativeAllocator logic minimal so that the test class can I don't think this belongs in the allocator class. The logic here is specific to the state of this nbaos (reads and modifies bufferPtr_). It also checks for BUFFER_MAX_SIZE_LIMIT which is not related to the allocator. I agree with Bharath and think it's simpler to keep the allocator a thin wrapper around the Unsafe calls. Pushing logic into the allocator means we need may need to duplicate it for the test mock (or otherwise share it). Maybe it's less confusing if we rename this function to growBuffer() or something. http://gerrit.cloudera.org:8080/#/c/7955/13/fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java File fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java: PS13, Line 43: return 0; > My point is that the allocator doesn't only return 0, is it? It could alway It can throw an OutOfMemory*Error* which is fatal. Are you saying we should try to recover from that? -- 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: 13 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
