Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-5881: Use native allocation while building catalog updates ......................................................................
Patch Set 9: (9 comments) http://gerrit.cloudera.org:8080/#/c/7955/9/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java: PS9, Line 67: . nit: remove PS9, Line 94: reAllocateMemory Unsafe.reallocateMemory() PS9, Line 159: public void writeTo(OutputStream out) { : throw new IllegalStateException("Not implemented."); : } : : public byte toByteArray()[] { : throw new IllegalStateException("Not implemented."); : } : : public int size() { : throw new IllegalStateException("Not implemented."); : } These are not defined in OutputStream, so what is the point of defining them here? Am I missing something? http://gerrit.cloudera.org:8080/#/c/7955/9/fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java File fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java: PS9, Line 38: @SuppressWarnings("restriction") explain? Also add a brief description of what this class tests. PS9, Line 78: huge "huge" doesn't really convey much information here. Maybe a bit more explicit what we're testing (>1GB, < 4GB)? Also, you may want to test multiple sizes to exercise all paths in the reallocation logic (e.g. 500MB, 1GB, 1.5GB, 2GB). You can decide which ones make more sense. PS9, Line 85: char[] chars = new char[128 * 1024 * 1024]; : Arrays.fill(chars, 'a'); : String hugeString = new String(chars); nit: see if you can use Guava's Strings.repeat() here, may be cheaper. PS9, Line 99: nit: extra space PS9, Line 99: Create a huge string of size 512MB. The combined size of the test object : // crosses 4GB. Maybe say something like "create an object with a serialization size which is larger than 4GB" PS9, Line 105: never ha, famous last words :) -- 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: 9 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
