Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-5881: Use native allocation while building catalog updates ......................................................................
Patch Set 16: (14 comments) http://gerrit.cloudera.org:8080/#/c/7955/16/be/src/catalog/catalog.cc File be/src/catalog/catalog.cc: Line 120: // Now that we deserialize the thrift objects into TGetAllCatalogObjectsResponse, > deserialized (past tense) Done http://gerrit.cloudera.org:8080/#/c/7955/16/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: Line 84: // flag. > remove this line Done http://gerrit.cloudera.org:8080/#/c/7955/16/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java: Line 61: " maximum limit of " + BUFFER_MAX_SIZE_LIMIT + " bytes"; > remove first space in string Done Line 99: * - bufferPtr_>=0 > use consistent spacing, i.e. bufferPtr_ >= 0 Done Line 102: Preconditions.checkState(bufferPtr_ >= 0); > remove (documenting in comment is better here) Done http://gerrit.cloudera.org:8080/#/c/7955/16/fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java File fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java: Line 28: // Custom NativeAllocator that accounts the allocated and freed bytes and randomly > the allocated/freed bytes and simulates allocation failures Done Line 39: // Throws an OOM on every alternate call to reallocate(). > on every 2nd call Done Line 51: allocatedBytes_ = 0; > single line Done Line 87: while (testAllocator_.getAllocationFailures() < 10) { > I still don't think this is a great approach because it does not reflect re Makes sense. Deterministic tests are always better and the test combinations are small too. Line 91: writeAndCheck(b, 1, 0); > These should be tested from a fresh Nbaos and not from the one that is alre yea I added a new nboas for all these set of tests. http://gerrit.cloudera.org:8080/#/c/7955/16/fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java File fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java: Line 58: // Deserialize the object at result.buffer_ptr and we confirm it is the same > remove "we" Done Line 119: Assert.assertTrue(nativeSerializer.getSerializedBytesSize() >= 3584 * 1024 * 1024); > Isn't there an assertGe() or something like that? Also print a message with Yea, I didn't find it. Line 137: e.printStackTrace(); > remove Done Line 166: testBasicSerialization(factory); > Let's make these separate top-level tests, if's fine to repeat the loop ove 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: 16 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
