impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] [PREVIEW] Use native allocation while building catalog updates
Date Wed, 06 Sep 2017 02:15:32 GMT
Bharath Vissapragada has posted comments on this change.

Change subject: [PREVIEW] Use native allocation while building catalog updates
......................................................................


Patch Set 1:

(17 comments)

http://gerrit.cloudera.org:8080/#/c/7955/1/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:

PS1, Line 106: RETURN_IF_ERROR(DeserializeThriftMsg(jni_env, result_bytes, &nbuffer));
> What happens if DeserializeThriftMsg returns a non-ok status? Who will rele
As we discussed, this is a tricky one. Reason being, if the call fails, it is likely that
nbuffer points to a non-existent memory and free'ing it can seg-fault the Catalog. I'm changing
it to an EXIT_IF_ERROR. Thoughts? cc: Alex.


PS1, Line 108: uint32_t len = static_cast<uint32_t>(nbuffer.buffer_len);
> Isn't buffer_len i64? What will happen if the cast fails?
Regarding i64, Yea, but thrift has some limits on how big a payload can be and hence all our
Deserialize*() methods accept a uint32_t sized length.

Regarding static cast failures, I've placed some limits on the allocator side to not exceed
that limit so that we fail fast on getCatalogObjects() and don't reach this point. Let me
know what you think.


PS1, Line 110: buf
> Interestingly, DeserializeThriftMsg() casts the const away of the first par
Checked the DeserializeThriftMsg, don't think thats possible.


PS1, Line 111: desrialize
> nit: deserialize (typo)
Done


PS1, Line 113: free(buf);
> Yes, I think we need to place it safe here. If the implementation changes i
Done


http://gerrit.cloudera.org:8080/#/c/7955/1/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:

PS1, Line 596: native byte buffer
> In the context of reading this thrift file, I don't think it is clear what 
Done


http://gerrit.cloudera.org:8080/#/c/7955/1/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:

Line 137:    * Gets all catalog objects
> Please expand the comment. We're adding some non-trivial behavior here.
Done


http://gerrit.cloudera.org:8080/#/c/7955/1/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:

Line 1: package org.apache.impala.thrift;
> Apache header?
Done


PS1, Line 26: @SuppressWarnings("restriction")
> explain?
looks like auto generated by IDE, not needed. Removed


PS1, Line 27: class
> nit: make it final (do you plan to extend it?)
Done


PS1, Line 33:   private static final long BUFFER_DOUBLING_RESIZE_LIMIT = 1 * 1024 * 1024 *
1024; /* 1GB */
> nit: long line
Done


PS1, Line 38: length
> length (in bytes)
Done


PS1, Line 39: protected
> why protected?
Done


PS1, Line 48: public NativeByteArrayOutputStream() {
            :     this(BUFFER_INITIAL_SIZE_DEFAULT);
            :   }
> single line?
Done


PS1, Line 57: // Unsafe#allocateMemory() can handle negative inputs.
> Why do I need to know about this here? Maybe remove.
I was doing some cleanup and added it. Doesn't make sense, can be removed. Appears totally
out of context in this CR, lol.


PS1, Line 94: if (bufferLen_ >= BUFFER_DOUBLING_RESIZE_LIMIT) {
            :         newBufferSize = bufferLen_ + BUFFER_RESIZE_INCREMENTS;
            :       } else {
            :         newBufferSize = bufferLen_ << 1;
            :       }
> How do you guarantee that newBufferSize > bytesWritten_ + len?
Yea thats a bug. fixed it.


http://gerrit.cloudera.org:8080/#/c/7955/1/fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
File fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java:

PS1, Line 31: native by array
> nit: "native by array"?
Done


-- 
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: 1
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