impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Date Fri, 08 Sep 2017 23:06:15 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................


Patch Set 13:

(8 comments)

Still looking at tests.

http://gerrit.cloudera.org:8080/#/c/7955/13/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:

Line 171:     LOG(ERROR) << "Failed to free buffers. Leaked memory of size: "
Failed to free buffer. (singular buffer)


http://gerrit.cloudera.org:8080/#/c/7955/13/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:

Line 155:         FileUtils.byteCountToDisplaySize(
We have a PrintUtils.printBytes(), use that for consistency. Maybe we should eventually remove
that in favor of  FileUtils.byteCountToDisplaySize() but not in this patch.


http://gerrit.cloudera.org:8080/#/c/7955/13/fe/src/main/java/org/apache/impala/thrift/NativeAllocator.java
File fe/src/main/java/org/apache/impala/thrift/NativeAllocator.java:

Line 23:  * methods.
Mention that we have this wrapper so we can override the methods for a testing.


Line 29:   public long allocate(long bufferPtr, long size) {
reallocate()


http://gerrit.cloudera.org:8080/#/c/7955/13/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:

Line 92:   public  NativeByteArrayOutputStream() { this(new NativeAllocator()); }
extra space after public


Line 103:       if (len <= 0) {
Is this dead code? Not sure we can ever get here (how would we write a test to cover this
code path?)


PS13, Line 100: private void tryAllocate(long len) {
              :     Preconditions.checkState(bufferPtr_ >= 0);
              :     try {
              :       if (len <= 0) {
              :         throw new IllegalArgumentException(INVALID_BUFFER_LEN_MSG + ": " +
len);
              :       }
              :       if (len >= BUFFER_MAX_SIZE_LIMIT) {
              :         throw new IllegalArgumentException(BUFFER_LIMIT_EXCEEDED_MSG + ":
" + len);
              :       }
              :       long newBufferPtr = nativeAllocator_.allocate(bufferPtr_, len);
              :       if (newBufferPtr == 0) {
              :         throw new IllegalStateException(ALLOCATION_FAILED_MSG + ": " + len
              :             + " Is realloc: " + (bufferPtr_ > 0));
              :       }
              :       bufferPtr_ = newBufferPtr;
              :       bufferLen_ = len;
              :     } catch (Throwable e) {
              :       nativeAllocator_.free(bufferPtr_);
              :       throw e;
              :     }
              :   }
> Wanted to keep the NativeAllocator logic minimal so that the test class can
I don't think this belongs in the allocator class. The logic here is specific to the state
of this nbaos (reads and modifies bufferPtr_). It also checks for BUFFER_MAX_SIZE_LIMIT which
is not related to the allocator.

I agree with Bharath and think it's simpler to keep the allocator a thin wrapper around the
Unsafe calls. Pushing logic into the allocator means we need may need to duplicate it for
the test mock (or otherwise share it).

Maybe it's less confusing if we rename this function to growBuffer() or something.


http://gerrit.cloudera.org:8080/#/c/7955/13/fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java
File fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java:

PS13, Line 43: return 0;
> My point is that the allocator doesn't only return 0, is it? It could alway
It can throw an OutOfMemory*Error* which is fatal. Are you saying we should try to recover
from that?


-- 
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: 13
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