Alex Behm has posted comments on this change. Change subject: IMPALA-5881: Use native allocation while building catalog updates ......................................................................
Patch Set 17: (7 comments) http://gerrit.cloudera.org:8080/#/c/7955/17/fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java File fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java: Line 40: || size >= NativeByteArrayOutputStream.BUFFER_MAX_SIZE_LIMIT) { Our real allocator would not OOM if the size is big, at least not in this place, so let's remove the second check. Line 60: private void writeAndCheck(NativeByteArrayOutputStream nboas, I'd prefer to split this up into writeOk() and writeNotOk(). Otherwise, tests may go into the "wrong" code path but still succeed, e.g., because the allocation succeeded and updated the bytesWritten correctly, although we expected the allocation to fail. Line 78: NativeByteArrayOutputStream nboas = The first allocation happens here in the c'tor. We need to test that as well. Alternatively, you can change the nbaos implementation to not allocate in the c'tor (either way is fine with me) Line 90: nboas = new NativeByteArrayOutputStream(testAllocator); nit: nbaos Line 108: remove extraneous newines Line 114: testAllocator = new NativeTestAllocator(); Add a helper function for these smaller tests that creates a new allocator, nbaos, array, etc. http://gerrit.cloudera.org:8080/#/c/7955/17/fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java File fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java: Line 81: public void testBasicSerialization() We usually call these TestBasicSerialization (first letter is uppercase) -- 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: 17 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
