impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <>
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:

File be/src/catalog/

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)

PS1, Line 113: free(buf);
> Yes, I think we need to place it safe here. If the implementation changes i
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 
File fe/src/main/java/org/apache/impala/service/

Line 137:    * Gets all catalog objects
> Please expand the comment. We're adding some non-trivial behavior here.
File fe/src/main/java/org/apache/impala/thrift/

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

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?)

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

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

PS1, Line 39: protected
> why protected?

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

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.
File fe/src/main/java/org/apache/impala/thrift/

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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Mostafa Mokhtar <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message