Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-5881: Use native allocation while building catalog updates ......................................................................
Patch Set 13: (6 comments) 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) Done 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 shoul I promise I searched for this, couldn't find it and hence used the FileUtils :) 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 test Done Line 29: public long allocate(long bufferPtr, long size) { > reallocate() Done 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 Done Line 103: if (len <= 0) { > Is this dead code? Not sure we can ever get here (how would we write a test Yes, this is dead code, just retained for completeness. Should I remove? don't think we can exercise this using tests. -- 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
