Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-5881: Use native allocation while building catalog updates ......................................................................
Patch Set 9: (14 comments) Redo'ing the test a little, will update the new PS shortly. http://gerrit.cloudera.org:8080/#/c/7955/8/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: PS8, Line 603: // Struct containing eight strings, used for testing. > Mention what are we testing with this weird looking struct. Also, leave a T Done http://gerrit.cloudera.org:8080/#/c/7955/8/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: PS8, Line 51: import org.apache.impala.thrift.TNativeByteBuffer; > dup (L49) Done 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 Done Line 87: private NativeByteArrayOutputStream(long initialSize) { > Merge into the public constructor to remove some code and never mention a c Done PS9, Line 94: reAllocateMemory > Unsafe.reallocateMemory() Done Line 99: Preconditions.checkState(bufferPtr_ >= 0); > needs to go above the try, otherwise we'll try to free an invalid ptr in L1 Done Line 104: throw new RuntimeException(BUFFER_LIMIT_EXCEEDED_MSG + ": " + len); > IllegalArgumentException? Done 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 the These are from ByteArrayOutputStream. Given we mimic it, someone might expect to call them. I'm fine with keeping/removing them. 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. IDE magic, removed PS9, Line 78: huge > "huge" doesn't really convey much information here. Maybe a bit more explic Done 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. Done PS9, Line 99: > nit: extra space Some kind of visual effect, don't think there are two spaces. 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 Done PS9, Line 105: never > ha, famous last words :) lol :D -- 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 <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
