impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Date Tue, 12 Sep 2017 22:01:42 GMT
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 <bharathv@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message