Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-5881: Use native allocation while building catalog updates ......................................................................
Patch Set 13: (2 comments) 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: 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; : } : } > I think that belongs to the Allocator class. Wanted to keep the NativeAllocator logic minimal so that the test class can override very little and the exceptions are thrown from NBAOS code. If we move this method there, we need to override and implement a very similar logic again (omitting the unsafe stuff). We can still do it by splitting the unsafe stuff into a different method, but I thought it isn't worth it just for tests. Thoughts? 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; > Shouldn't the allocate throw an OutOfMemoryException as well? Not sure I understand, NBAOS.tryAllocate() reads this 0 and throws IllegalStateException() that we catch? -- 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 <[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
