Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-5881: Use native allocation while building catalog updates ......................................................................
Patch Set 18: (8 comments) http://gerrit.cloudera.org:8080/#/c/7955/18/be/src/catalog/catalog.cc File be/src/catalog/catalog.cc: PS18, Line 117: DCHECK_LE(len, numeric_limits<uint32_t>::max()); > This DCHECK needs to be run against nbuffer.buffer_len directly. I'm not totally sure about this part, but looks like the static_cast() can silently fail. Either way I think its good to make this check on buffer_len http://gerrit.cloudera.org:8080/#/c/7955/18/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: PS18, Line 155: Catalog > nit: catalog Done http://gerrit.cloudera.org:8080/#/c/7955/18/fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java File fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java: Line 61: nbaos.write(b, offset, len); > try catch and assert if we catch an exception or use the exceptionCaught pa As discussed, not really needed since we expect the write to throw if an error occurs and then it is caught by the test. Line 67: // Makes sure that the memory is freed by the end of nboas.write(). > Expects a write() to fail and checks that the memory is freed after the uns Done Line 71: nbaos.write(b, offset, len); > Add an assert that fails if the write succeeded. Done Line 95: public void nbaosTest() { > NbaosTest Done Line 99: testAllocator = new NativeTestAllocator(); > move into L96 Done Line 100: // Check that initial allocation failure in NBAOS c'tor > remove "that" 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: 18 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
