Alex Behm has posted comments on this change. Change subject: IMPALA-5881: Use native allocation while building catalog updates ......................................................................
Patch Set 1: (4 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)); > As we discussed, this is a tricky one. Reason being, if the call fails, it We should check that the nbuffer ptr and size are not 0. I don't think it should be possible to pass in a non-zero but invalid ptr from nbuffer. The FE code must ensure that only valid pointers (nullptr or real memory) are returned. If the deserialization fails, I think we should free the native memory and return a status - assuming this state is probably recoverable in the next topic update. 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 89: throw new IndexOutOfBoundsException( only if bufferPtr_ != 0 Line 109: If this function throws we'll have a memory leak. We could either internally ensure that whenever an exception is thrown all native memory is freed, or we force the caller to try/catch and free the memory. I'd prefer the former behavior. http://gerrit.cloudera.org:8080/#/c/7955/4/fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java File fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java: Line 31: * native by array using NativeByteArrayOutputStream class. Not thread-safe. Mention that it is valid to call serialize() exactly once (maybe we can enforce that with a flag), regardless of whether serialize() succeeded or failed with an exception. -- 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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
