impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Date Wed, 06 Sep 2017 15:36:34 GMT
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 <bharathv@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message