Alex Behm has posted comments on this change. Change subject: IMPALA-5881: Use native allocation while building catalog updates ......................................................................
Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/7955/4/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: Line 597: // struct tracks the buffer's pointer and length. Currently used by the Catalog JVM I don't think we need the last part, comments about current usage tend to get stale fast. Describing what this struct represents is enough. http://gerrit.cloudera.org:8080/#/c/7955/4/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: Line 112: Preconditions.checkNotNull(catalogUpdateProtocolFactory_); seems clear it cannot be null here Line 153: if (serializedByteSize >= SERIALIZED_TOPIC_SIZE_WARNING_LIMIT_BYTES) { What's the purpose of this warning? I'm thinking we should address limits/warnings in a follow-on change like Juan suggested. http://gerrit.cloudera.org:8080/#/c/7955/4/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java: Line 28: * implements a single write() method that automatically resizes ... single write() method because that is used by the thrift-generated serialization code. Line 43: public final class NativeByteArrayOutputStream extends OutputStream { We should add unit tests for this class and the TNativeSerializer. Line 94: bufferPtr_ = UnsafeUtil.UNSAFE.reallocateMemory(bufferPtr_, len); My understanding is that if reallocate() fails (return 0), then the original buffer is not freed, so you need to take care of that (my prototype handled that case) so as to not leak memory. -- 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: 4 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
