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