You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org> on 2017/09/05 03:12:41 UTC
[Impala-ASF-CR] [PREVIEW] Use native allocation while building catalog updates
Bharath Vissapragada has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7955
Change subject: [PREVIEW] Use native allocation while building catalog updates
......................................................................
[PREVIEW] Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
(Still trying to scale up the catalog topic to go beyond 4-5GB
uncompressed topics).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
8 files changed, 268 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/1
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 18: Code-Review+1
(6 comments)
Dimitris, any more comments?
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 pattern you use for the c'tor test below
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 unsuccessful write().
Line 71: nbaos.write(b, offset, len);
Add an assert that fails if the write succeeded.
Line 95: public void nbaosTest() {
NbaosTest
Line 99: testAllocator = new NativeTestAllocator();
move into L96
Line 100: // Check that initial allocation failure in NBAOS c'tor
remove "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: 18
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 8:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/7955/8/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:
Line 116: DCHECK(len < std::numeric_limits<uint32_t>::max());
DCHECK_LE
#include <limit>
Line 170: LOG(ERROR) << "Failed to free buffers. Potential memory leak of: "
Failed to free native buffer through JNI. Leaked N bytes.
(We definitely leaked at this point).
Also include the status error message.
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:
Line 295: TException {
Does this really throw TException?
http://gerrit.cloudera.org:8080/#/c/7955/8/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:
Line 94: private void tryAllocate(long len, boolean reAllocate) {
Can we simplify this to just always use reallocateMemory()? Realloc from a 0 ptr should be ok I think.
Line 95: try {
Preconditions.checkState(bufferPtr_ >= 0);
for clarity why the free in L114 is always ok
Line 113: } catch (Exception e) {
catch Throwable for extra paranoia
Line 139: if (newBufferSize >= BUFFER_MAX_SIZE_LIMIT) {
|| newBufferSize < 0 (overflow)
Line 155: public synchronized void reset() {}
What's this for? Is it an @Override?
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#18).
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
IMPALA-5881: Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
This patch also caps the native allocations to not go beyond 4GB due
to Thrift library limitations. (Thrift internally uses uint32_t
datatype to represent the message size which limits the size to
~4.2GB).
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeAllocator.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
A fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java
A fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
12 files changed, 731 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/18
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 18
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 14:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/7955/14/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:
Line 110: if (newBufferPtr == 0) {
> I think it is better to retain the check, just in case the JVM behavior cha
Makes sense
Line 128: if ((offset < 0) || (offset > b.length) || (len < 0)
if (b == null) return;
http://gerrit.cloudera.org:8080/#/c/7955/14/fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java
File fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java:
Line 42: && (rand_.nextInt(10) < 8)) {
Let's avoid non-determinism, otherwise a test failure may be hard to debug (how to repro?).
How about we just keep a long counter and fail whenever counter % 2 == 0 or something along those lines.
Even using a fixed random seed seems unnecessarily complex.
Line 89: while (testAllocator_.getAllocationFailures() < 10) {
Not a big fan of the non-determinism. How about we run this loop a fixed (but large) number of times instead. Non-determinism will be hard to debug and give unpredictable test runtimes.
Line 91: byte[] b = new byte[byteArraySize];
Can we get away with allocating a single array of size byteArrayMaxSize?
Line 92: writeAndCheck(b, 0, byteArraySize);
How does this work? Is the NBAOS expected to be left in a good state after write() throws an OOM? I think this test "quietly" assumes we are using the NativeTestAllocator, and it would not work with the real NativeAllocator (because we'd try to realloc a garbage pointer).
Perhaps we should make the TestAllocator more real by actually allocating/freeing/copying
Line 97: writeAndCheck(b, -1, 10);
Test that len == 0 works
Line 100: writeAndCheck(b, 5, 10);
Test passing a null byte array
http://gerrit.cloudera.org:8080/#/c/7955/14/fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
File fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java:
Line 74: UnsafeUtil.UNSAFE.freeMemory(result.buffer_ptr);
put in finally block
Line 86: * buffer resizing capability in the underlying NativeByteArrayOutputStreami and
typo: extraneous 'i' after NBAOS
Line 107: serializeTestObject(test15gb, protocolFactory);
we also need to test the max 4GB
Line 119: // The reason it is not a problem with Impala's catalog updates is because a single
I'd remove this sentence, it's not really true.
Line 139: }
Test that calling serialize() twice results in an exception.
--
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: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 8:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/7955/8/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:
Line 116: DCHECK(len < std::numeric_limits<uint32_t>::max());
> DCHECK_LE
Done
Line 170: LOG(ERROR) << "Failed to free buffers. Potential memory leak of: "
> Failed to free native buffer through JNI. Leaked N bytes.
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:
Line 295: TException {
> Does this really throw TException?
good catch, no!
http://gerrit.cloudera.org:8080/#/c/7955/8/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:
Line 94: private void tryAllocate(long len, boolean reAllocate) {
> Can we simplify this to just always use reallocateMemory()? Realloc from a
Good point, checked the JVM sources, it already includes the check to call malloc/realloc depending on whether we pass 0.
http://hg.openjdk.java.net/jdk8/jdk8/hotspot/file/87ee5ee27509/src/share/vm/prims/unsafe.cpp#l613
Line 95: try {
> Preconditions.checkState(bufferPtr_ >= 0);
Done
Line 113: } catch (Exception e) {
> catch Throwable for extra paranoia
Done
Line 139: if (newBufferSize >= BUFFER_MAX_SIZE_LIMIT) {
> || newBufferSize < 0 (overflow)
Don't think thats possible given we cap at BUFFER_MAX_SIZE_LIMIT which is <<< long_max. I'm still including it to be on the safe side.
Line 155: public synchronized void reset() {}
> What's this for? Is it an @Override?
Not an ovveride, removed, this was actually being called from serializer, but does nothing.
Ideally useful to reset the state of NBOAS, but given we don't allow calling serialize twice, I don't think that is necessary.
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#6).
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
IMPALA-5881: Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
This patch also caps the native allocations to not go beyond 4GB due
to Thrift library limitations. (Thrift internally uses uint32_t
datatype to represent the message size which limits the size to
~4.2GB).
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
A fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
10 files changed, 461 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/6
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 4:
(2 comments)
Responding to comments, will look at code next.
http://gerrit.cloudera.org:8080/#/c/7955/1/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:
PS1, Line 106: TNativeByteBuffer nbuffer;
> I think the problem is when nbuffer_ptr is non zero but DeSerialize call fa
I'm not really sure what error scenario you are worried about. We allocate the pointer with malloc(). If we return from DeserializeThriftMsg() with a non-OK status, I don't think we can definitely conclude that we got a bad pointer.
I'm thinking that DeserializeThriftMsg() could fail due to a bug in thrift (or other recoverable reasons?), and that the next time around deserialization will just work with a different payload.
http://gerrit.cloudera.org:8080/#/c/7955/4/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
Line 153: if (serializedByteSize >= SERIALIZED_TOPIC_SIZE_WARNING_LIMIT_BYTES) {
> I included this for supportability purposes. With this we log the untracked
I'm not opposed to adding warnings, just concerned that users will worry about them and not know how to fix them. Let me think about this a little more.
I'm also wondering what we could actually do with this information - the next question would probably be a breakdown of why the topic is so big.
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7955/1/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:
PS1, Line 106: RETURN_IF_ERROR(DeserializeThriftMsg(jni_env, result_bytes, &nbuffer));
> As we discussed, this is a tricky one. Reason being, if the call fails, it
We should check that the nbuffer ptr and size are not 0.
I don't think it should be possible to pass in a non-zero but invalid ptr from nbuffer. The FE code must ensure that only valid pointers (nullptr or real memory) are returned. If the deserialization fails, I think we should free the native memory and return a status - assuming this state is probably recoverable in the next topic update.
http://gerrit.cloudera.org:8080/#/c/7955/4/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:
Line 89: throw new IndexOutOfBoundsException(
only if bufferPtr_ != 0
Line 109:
If this function throws we'll have a memory leak. We could either internally ensure that whenever an exception is thrown all native memory is freed, or we force the caller to try/catch and free the memory. I'd prefer the former behavior.
http://gerrit.cloudera.org:8080/#/c/7955/4/fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
File fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java:
Line 31: * native by array using NativeByteArrayOutputStream class. Not thread-safe.
Mention that it is valid to call serialize() exactly once (maybe we can enforce that with a flag), regardless of whether serialize() succeeded or failed with an exception.
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 13:
PS 13 adds more unit tests the NBAOS.
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 17:
(7 comments)
Fixed a subtle bug in the test allocator.
http://gerrit.cloudera.org:8080/#/c/7955/17/fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java
File fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java:
Line 40: || size >= NativeByteArrayOutputStream.BUFFER_MAX_SIZE_LIMIT) {
> Our real allocator would not OOM if the size is big, at least not in this p
Done
Line 60: private void writeAndCheck(NativeByteArrayOutputStream nboas,
> I'd prefer to split this up into writeOk() and writeNotOk(). Otherwise, tes
Done
Line 78: NativeByteArrayOutputStream nboas =
> The first allocation happens here in the c'tor. We need to test that as wel
Done
Line 90: nboas = new NativeByteArrayOutputStream(testAllocator);
> nit: nbaos
Done
Line 108:
> remove extraneous newines
Done
Line 114: testAllocator = new NativeTestAllocator();
> Add a helper function for these smaller tests that creates a new allocator,
Done
http://gerrit.cloudera.org:8080/#/c/7955/17/fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
File fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java:
Line 81: public void testBasicSerialization()
> We usually call these TestBasicSerialization (first letter is uppercase)
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: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#11).
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
IMPALA-5881: Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
This patch also caps the native allocations to not go beyond 4GB due
to Thrift library limitations. (Thrift internally uses uint32_t
datatype to represent the message size which limits the size to
~4.2GB).
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
A fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
10 files changed, 493 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/11
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] [PREVIEW] Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: [PREVIEW] Use native allocation while building catalog updates
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7955/1/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:
PS1, Line 113: free(buf);
> This freaks me out a bit. Any documentation I've read says that you should
I used free after checking that the unsafe.freeMemory() also calls the same.
http://hg.openjdk.java.net/jdk8/jdk8/hotspot/file/87ee5ee27509/src/share/vm/prims/unsafe.cpp#l620
Do you think we should make a JNI call to the JVM and make it call the Unsafe#freeMemory() instead?
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 1:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/7955/1/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:
PS1, Line 106: RETURN_IF_ERROR(DeserializeThriftMsg(jni_env, result_bytes, &nbuffer));
> We should check that the nbuffer ptr and size are not 0.
I think the problem is when nbuffer_ptr is non zero but DeSerialize call fails (free(0) works ok). Per my understanding there is no proper way to confirm that it points to a correct address. Do you suggest that we should free it in any case and return the status?
http://gerrit.cloudera.org:8080/#/c/7955/4/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:
Line 597: // Currently used by the Catalog JVM to serialize and track the output of
> I don't think we need the last part, comments about current usage tend to g
Done
http://gerrit.cloudera.org:8080/#/c/7955/4/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
Line 112: GlogAppender.Install(TLogLevel.values()[cfg.impala_log_lvl],
> seems clear it cannot be null here
Done
Line 153: /**
> What's the purpose of this warning? I'm thinking we should address limits/w
I included this for supportability purposes. With this we log the untracked allocation sizes and also gives a rough estimate of the topic size. Do you think it should be removed?
For ex: On a cluster where I deployed this page, this line got printed for a a serialized size of 2.5GB. That way I figured that a single call got a huge update from the Catalog, something along these lines.
http://gerrit.cloudera.org:8080/#/c/7955/4/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:
Line 28:
> ... single write() method because that is used by the thrift-generated seri
Done
Line 43: protected long bytesWritten_;
> We should add unit tests for this class and the TNativeSerializer.
Added a single unit test class that tests both. Running into some issues while its execution because of huge allocations. Commented the relevant portions while I'm figuring out an optimal way to do it.
Line 89: throw new IndexOutOfBoundsException(
> only if bufferPtr_ != 0
I checked that freeMemory(0) is a valid call, wrote a test program to confirm that.
Line 94: if (bufferLen_ >= BUFFER_DOUBLING_RESIZE_LIMIT) {
> My understanding is that if reallocate() fails (return 0), then the origina
Yea. I added a freeMemory() if the return is 0.
Line 109:
> If this function throws we'll have a memory leak. We could either internall
The only method that can throw here is tryAllocate() and I added necessary cleanups there.
(I checked that the copyMemory doesn't throw).
http://gerrit.cloudera.org:8080/#/c/7955/4/fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
File fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java:
Line 31: * native by array using NativeByteArrayOutputStream class. Not thread-safe.
> Mention that it is valid to call serialize() exactly once (maybe we can enf
Mentioned it in the serialize() method comment.
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#4).
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
IMPALA-5881: Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
This patch also caps the native allocations to not go beyond 4GB due
to Thrift library limitations. (Thrift internally uses uint32_t
datatype to represent the message size which limits the size to
~4.2GB).
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
9 files changed, 312 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/4
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 13:
(1 comment)
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 103: if (len <= 0) {
> Yes, this is dead code, just retained for completeness. Should I remove? do
I think it's better to remove and document the preconditions this function in the comment (there's no point in adding a Preconditions check which will only waste cycles for something that can never happen)
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7955
to look at the new patch set (#19).
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
IMPALA-5881: Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
This patch also caps the native allocations to not go beyond 4GB due
to Thrift library limitations. (Thrift internally uses uint32_t
datatype to represent the message size which limits the size to
~4.2GB).
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeAllocator.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
A fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java
A fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
12 files changed, 731 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/19
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 8:
(2 comments)
Flushing minor comments from patch #8 and switching to patch #9.
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 TODO to move this out of here.
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)
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 9:
(3 comments)
Code looks good to me, still need to flesh out tests.
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:
Line 87: private NativeByteArrayOutputStream(long initialSize) {
Merge into the public constructor to remove some code and never mention a custom initial capacity (to keep things simple).
Line 99: Preconditions.checkState(bufferPtr_ >= 0);
needs to go above the try, otherwise we'll try to free an invalid ptr in L113
Line 104: throw new RuntimeException(BUFFER_LIMIT_EXCEEDED_MSG + ": " + len);
IllegalArgumentException?
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 13:
(6 comments)
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)
Done
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 shoul
I promise I searched for this, couldn't find it and hence used the FileUtils :)
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 test
Done
Line 29: public long allocate(long bufferPtr, long size) {
> reallocate()
Done
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
Done
Line 103: if (len <= 0) {
> Is this dead code? Not sure we can ever get here (how would we write a test
Yes, this is dead code, just retained for completeness. Should I remove? don't think we can exercise this using tests.
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7955/4/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:
Line 597: // struct tracks the buffer's pointer and length. Currently used by the Catalog JVM
I don't think we need the last part, comments about current usage tend to get stale fast. Describing what this struct represents is enough.
http://gerrit.cloudera.org:8080/#/c/7955/4/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
Line 112: Preconditions.checkNotNull(catalogUpdateProtocolFactory_);
seems clear it cannot be null here
Line 153: if (serializedByteSize >= SERIALIZED_TOPIC_SIZE_WARNING_LIMIT_BYTES) {
What's the purpose of this warning? I'm thinking we should address limits/warnings in a follow-on change like Juan suggested.
http://gerrit.cloudera.org:8080/#/c/7955/4/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:
Line 28: * implements a single write() method that automatically resizes
... single write() method because that is used by the thrift-generated serialization code.
Line 43: public final class NativeByteArrayOutputStream extends OutputStream {
We should add unit tests for this class and the TNativeSerializer.
Line 94: bufferPtr_ = UnsafeUtil.UNSAFE.reallocateMemory(bufferPtr_, len);
My understanding is that if reallocate() fails (return 0), then the original buffer is not freed, so you need to take care of that (my prototype handled that case) so as to not leak memory.
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#7).
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
IMPALA-5881: Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
This patch also caps the native allocations to not go beyond 4GB due
to Thrift library limitations. (Thrift internally uses uint32_t
datatype to represent the message size which limits the size to
~4.2GB).
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
A fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
10 files changed, 461 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/7
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#12).
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
IMPALA-5881: Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
This patch also caps the native allocations to not go beyond 4GB due
to Thrift library limitations. (Thrift internally uses uint32_t
datatype to represent the message size which limits the size to
~4.2GB).
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeAllocator.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
A fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java
A fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
12 files changed, 648 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/12
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 13:
(1 comment)
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;
> It can throw an OutOfMemory*Error* which is fatal. Are you saying we should
It is but we're catching it and trying to release resources. We need to make sure that this path works as expected. Right?
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 13:
(2 comments)
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:
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;
: }
: }
I think that belongs to the Allocator class.
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;
Shouldn't the allocate throw an OutOfMemoryException as well?
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 18: Code-Review+2
(2 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());
Is this actually needed? Isn't the cast in the line above going to fail if the condition in the DCHECK is false?
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
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#2).
Change subject: [PREVIEW] Use native allocation while building catalog updates
......................................................................
[PREVIEW] Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
(Still trying to scale up the catalog topic to go beyond 4-5GB
uncompressed topics).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
9 files changed, 315 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/2
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] [PREVIEW] Use native allocation while building catalog updates
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: [PREVIEW] Use native allocation while building catalog updates
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7955/1/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:
PS1, Line 113: free(buf);
> I used free after checking that the unsafe.freeMemory() also calls the same
Switching the JVM to allocate huge chunks of memory via malloc is going to have a bunch of consequences - adding a potentially large amount of untracked non-JVM memory.
I strongly feel that we shouldn't change the default behaviour here.
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: [PREVIEW] Use native allocation while building catalog updates
......................................................................
Patch Set 1:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/7955/1/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:
PS1, Line 106: RETURN_IF_ERROR(DeserializeThriftMsg(jni_env, result_bytes, &nbuffer));
> What happens if DeserializeThriftMsg returns a non-ok status? Who will rele
As we discussed, this is a tricky one. Reason being, if the call fails, it is likely that nbuffer points to a non-existent memory and free'ing it can seg-fault the Catalog. I'm changing it to an EXIT_IF_ERROR. Thoughts? cc: Alex.
PS1, Line 108: uint32_t len = static_cast<uint32_t>(nbuffer.buffer_len);
> Isn't buffer_len i64? What will happen if the cast fails?
Regarding i64, Yea, but thrift has some limits on how big a payload can be and hence all our Deserialize*() methods accept a uint32_t sized length.
Regarding static cast failures, I've placed some limits on the allocator side to not exceed that limit so that we fail fast on getCatalogObjects() and don't reach this point. Let me know what you think.
PS1, Line 110: buf
> Interestingly, DeserializeThriftMsg() casts the const away of the first par
Checked the DeserializeThriftMsg, don't think thats possible.
PS1, Line 111: desrialize
> nit: deserialize (typo)
Done
PS1, Line 113: free(buf);
> Yes, I think we need to place it safe here. If the implementation changes i
Done
http://gerrit.cloudera.org:8080/#/c/7955/1/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:
PS1, Line 596: native byte buffer
> In the context of reading this thrift file, I don't think it is clear what
Done
http://gerrit.cloudera.org:8080/#/c/7955/1/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
Line 137: * Gets all catalog objects
> Please expand the comment. We're adding some non-trivial behavior here.
Done
http://gerrit.cloudera.org:8080/#/c/7955/1/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:
Line 1: package org.apache.impala.thrift;
> Apache header?
Done
PS1, Line 26: @SuppressWarnings("restriction")
> explain?
looks like auto generated by IDE, not needed. Removed
PS1, Line 27: class
> nit: make it final (do you plan to extend it?)
Done
PS1, Line 33: private static final long BUFFER_DOUBLING_RESIZE_LIMIT = 1 * 1024 * 1024 * 1024; /* 1GB */
> nit: long line
Done
PS1, Line 38: length
> length (in bytes)
Done
PS1, Line 39: protected
> why protected?
Done
PS1, Line 48: public NativeByteArrayOutputStream() {
: this(BUFFER_INITIAL_SIZE_DEFAULT);
: }
> single line?
Done
PS1, Line 57: // Unsafe#allocateMemory() can handle negative inputs.
> Why do I need to know about this here? Maybe remove.
I was doing some cleanup and added it. Doesn't make sense, can be removed. Appears totally out of context in this CR, lol.
PS1, Line 94: if (bufferLen_ >= BUFFER_DOUBLING_RESIZE_LIMIT) {
: newBufferSize = bufferLen_ + BUFFER_RESIZE_INCREMENTS;
: } else {
: newBufferSize = bufferLen_ << 1;
: }
> How do you guarantee that newBufferSize > bytesWritten_ + len?
Yea thats a bug. fixed it.
http://gerrit.cloudera.org:8080/#/c/7955/1/fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
File fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java:
PS1, Line 31: native by array
> nit: "native by array"?
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 13:
(1 comment)
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
Oh, now I see what you are saying. Thanks for explaining. I was under the impression that the reAllocate() silently returns 0, but I just checked the unsafe code and it actually throws the OOM. Made the changes accordingly.
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 15:
(1 comment)
The test as such in current state is susceptible to OOM, given we are doing huge allocations. I'm figuring out a way to workaround that so that it is not flaky. Meanwhile pushing out the test for review.
http://gerrit.cloudera.org:8080/#/c/7955/15/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:
PS15, Line 103: System.out.println(len);
Removed.
--
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: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 18:
Taking a look now.
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has abandoned this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Abandoned
While this change works as expected, I think we can be more uniform in our approach so that the system is easy to reason about. With this patch, its possible that the Catalog can support large topic size while Impalads can't deserialize it (for example, a single table larger than 2GB). While we explore other options, I'm abandoning this change.
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#15).
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
IMPALA-5881: Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
This patch also caps the native allocations to not go beyond 4GB due
to Thrift library limitations. (Thrift internally uses uint32_t
datatype to represent the message size which limits the size to
~4.2GB).
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeAllocator.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
A fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java
A fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
12 files changed, 673 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/15
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] Use native allocation while building catalog updates
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: [PREVIEW] Use native allocation while building catalog updates
......................................................................
Patch Set 1:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/7955/1/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:
PS1, Line 106: RETURN_IF_ERROR(DeserializeThriftMsg(jni_env, result_bytes, &nbuffer));
What happens if DeserializeThriftMsg returns a non-ok status? Who will release the allocated memory?
PS1, Line 108: uint32_t len = static_cast<uint32_t>(nbuffer.buffer_len);
Isn't buffer_len i64? What will happen if the cast fails?
PS1, Line 110: buf
Interestingly, DeserializeThriftMsg() casts the const away of the first param (buf). The comment in this function still says that it is treated as a const even though the const is casted away but let's make sure we check the implementation to ensure nothing bad happens to buf after that call.
PS1, Line 111: desrialize
nit: deserialize (typo)
PS1, Line 113: free(buf);
> I used free after checking that the unsafe.freeMemory() also calls the same
Yes, I think we need to place it safe here. If the implementation changes in some later Java version, we may run into issues.
http://gerrit.cloudera.org:8080/#/c/7955/1/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:
PS1, Line 596: native byte buffer
In the context of reading this thrift file, I don't think it is clear what "native byte buffer" is. Maybe say at the beginning that it represents a buffer allocated by the JVM using native memory (unsafe).
http://gerrit.cloudera.org:8080/#/c/7955/1/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
Line 137: * Gets all catalog objects
Please expand the comment. We're adding some non-trivial behavior here.
http://gerrit.cloudera.org:8080/#/c/7955/1/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:
Line 1: package org.apache.impala.thrift;
Apache header?
PS1, Line 26: @SuppressWarnings("restriction")
explain?
PS1, Line 27: class
nit: make it final (do you plan to extend it?)
PS1, Line 33: private static final long BUFFER_DOUBLING_RESIZE_LIMIT = 1 * 1024 * 1024 * 1024; /* 1GB */
nit: long line
PS1, Line 38: length
length (in bytes)
PS1, Line 39: protected
why protected?
PS1, Line 48: public NativeByteArrayOutputStream() {
: this(BUFFER_INITIAL_SIZE_DEFAULT);
: }
single line?
PS1, Line 57: // Unsafe#allocateMemory() can handle negative inputs.
Why do I need to know about this here? Maybe remove.
PS1, Line 94: if (bufferLen_ >= BUFFER_DOUBLING_RESIZE_LIMIT) {
: newBufferSize = bufferLen_ + BUFFER_RESIZE_INCREMENTS;
: } else {
: newBufferSize = bufferLen_ << 1;
: }
How do you guarantee that newBufferSize > bytesWritten_ + len?
E.g. bufferLen_ = 128MB, len = 1GB. Isn't bufferLen_ going to be 256MB although you need 1128MB? Am I missing something?
http://gerrit.cloudera.org:8080/#/c/7955/1/fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
File fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java:
PS1, Line 31: native by array
nit: "native by array"?
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 19: Code-Review+2
Thanks for the reviews. Carrying +2.
--
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: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 18:
(1 comment)
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());
> Is this actually needed? Isn't the cast in the line above going to fail if
This DCHECK needs to be run against nbuffer.buffer_len directly.
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 13:
(2 comments)
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:
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;
: }
: }
> I think that belongs to the Allocator class.
Wanted to keep the NativeAllocator logic minimal so that the test class can override very little and the exceptions are thrown from NBAOS code.
If we move this method there, we need to override and implement a very similar logic again (omitting the unsafe stuff). We can still do it by splitting the unsafe stuff into a different method, but I thought it isn't worth it just for tests. Thoughts?
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;
> Shouldn't the allocate throw an OutOfMemoryException as well?
Not sure I understand, NBAOS.tryAllocate() reads this 0 and throws IllegalStateException() that we catch?
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] Use native allocation while building catalog updates
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: [PREVIEW] Use native allocation while building catalog updates
......................................................................
Patch Set 1:
Alex explained to me that this doesn't actually increase the peak memory usage, since it never overlaps with the lifetime of the std::string containing the same data. So seems ok to me.
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 9:
(1 comment)
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 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.");
: }
> I get that, but you're not extending the ByteArrayOutputStream class, you'r
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#9).
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
IMPALA-5881: Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
This patch also caps the native allocations to not go beyond 4GB due
to Thrift library limitations. (Thrift internally uses uint32_t
datatype to represent the message size which limits the size to
~4.2GB).
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
A fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
10 files changed, 498 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/9
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] [PREVIEW] Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: [PREVIEW] Use native allocation while building catalog updates
......................................................................
Patch Set 1:
Thanks Alex and Tim. Yes, the peak memory usage shouldn't change with this patch. However, you raised a very good point that we need to track the memory better, however short-lived it is.
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 14:
(13 comments)
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 103: if (len <= 0) {
> I think it's better to remove and document the preconditions this function
Removed.
http://gerrit.cloudera.org:8080/#/c/7955/14/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:
Line 128: if ((offset < 0) || (offset > b.length) || (len < 0)
> if (b == null) return;
Done
http://gerrit.cloudera.org:8080/#/c/7955/14/fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java
File fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java:
Line 42: && (rand_.nextInt(10) < 8)) {
> Let's avoid non-determinism, otherwise a test failure may be hard to debug
Fair point, switched to the counter logic.
Line 89: while (testAllocator_.getAllocationFailures() < 10) {
> Not a big fan of the non-determinism. How about we run this loop a fixed (b
Now that the above logic is switched to a counter based one. this loop is deterministic as well.
Line 91: byte[] b = new byte[byteArraySize];
> Can we get away with allocating a single array of size byteArrayMaxSize?
Wanted to check varied allocation sizes, but other tests already do it, so I'm moving to a single size. I"m using 1MB rather than 1 GB.
Line 92: writeAndCheck(b, 0, byteArraySize);
> How does this work? Is the NBAOS expected to be left in a good state after
Yea this only runs because the write actually doesn't modify the underlying nboas, due to the TestAllocator. Please correct me if I'm wrong, but isn't the unit test for checking that we free in all edge cases? TNativeSerialzer test already writes multiple sized objects that does the underlying allocate/free/copying?
Line 97: writeAndCheck(b, -1, 10);
> Test that len == 0 works
Done
Line 100: writeAndCheck(b, 5, 10);
> Test passing a null byte array
Done
http://gerrit.cloudera.org:8080/#/c/7955/14/fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
File fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java:
Line 74: UnsafeUtil.UNSAFE.freeMemory(result.buffer_ptr);
> put in finally block
Done
Line 86: * buffer resizing capability in the underlying NativeByteArrayOutputStreami and
> typo: extraneous 'i' after NBAOS
Done
Line 107: serializeTestObject(test15gb, protocolFactory);
> we also need to test the max 4GB
Added a test for ~3.5GB, made sure we serialize it ok, as we can't deserialize it on the Java side. However it runs into occassional OOMs due to memory pressure. Still figuring out a way to fix that. May be run testThriftLimits for only one protocol?
Line 119: // The reason it is not a problem with Impala's catalog updates is because a single
> I'd remove this sentence, it's not really true.
lol :) Done.
Line 139: }
> Test that calling serialize() twice results in an exception.
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: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 13:
(1 comment)
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;
> Not sure I understand, NBAOS.tryAllocate() reads this 0 and throws IllegalS
My point is that the allocator doesn't only return 0, is it? It could always throw an OutOfMemoryException which the tryAllocate will catch, try to free any allocated memory and then re-throw. Shouldn't we be testing that path as well?
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] Use native allocation while building catalog updates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: [PREVIEW] Use native allocation while building catalog updates
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7955/1/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:
PS1, Line 113: free(buf);
> Switching the JVM to allocate huge chunks of memory via malloc is going to
1. This code path is catalogd only.
2. Roughly the same amount of untracked memory is already allocated today when creating the thrift topic update to the statestore (data lives in std::string). Imo, this patch makes the untracked memory problem no worse than it already is - fair point that it does not make it better either.
3. The 2GB JVM array limit means the catalogd goes down for large catalogs, and is very hard to recover, so we really need to solve this issue. I can imagine alternative solutions that involve copying large amounts of data back-and-forth through JNI (e.g. implementing a chunked ByteArrayOutputStream)
Do you have an alternative proposal?
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#14).
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
IMPALA-5881: Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
This patch also caps the native allocations to not go beyond 4GB due
to Thrift library limitations. (Thrift internally uses uint32_t
datatype to represent the message size which limits the size to
~4.2GB).
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeAllocator.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
A fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java
A fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
12 files changed, 649 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/14
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] [PREVIEW] Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#3).
Change subject: [PREVIEW] Use native allocation while building catalog updates
......................................................................
[PREVIEW] Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
(Still trying to scale up the catalog topic to go beyond 4-5GB
uncompressed topics).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
9 files changed, 315 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/3
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] [PREVIEW] Use native allocation while building catalog updates
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: [PREVIEW] Use native allocation while building catalog updates
......................................................................
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/7955/3//COMMIT_MSG
Commit Message:
PS3, Line 20: 4-5GB
You need to comment about the limit (4GB) that currently thrift is imposing.
http://gerrit.cloudera.org:8080/#/c/7955/3/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
PS3, Line 142: This method serializes
: * the output of CatalogService#getCatallogObjects() into native memory allocated
: * using a custom TNativeSerializer and returns the corresponding TNativeByteBuffer
: * serialized into bytes.
Alternatively: "Uses a custom serializer (TNativeSerializer) to serialize the catalog objects into a byte buffer that is backed by native memory."
http://gerrit.cloudera.org:8080/#/c/7955/3/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:
PS3, Line 36: 512MB
update value
Line 54: // Limit on the size to which the underlying buffer can grow
Comment why that limit exists
PS3, Line 103: off
offset
PS3, Line 115: if (bufferLen_ >= BUFFER_DOUBLING_RESIZE_LIMIT) {
: while (newBufferSize < bytesWritten_ + len) {
: newBufferSize += BUFFER_RESIZE_INCREMENTS;
: }
: } else {
: while (newBufferSize < bytesWritten_ + len) {
: newBufferSize <<= 1;
: }
: }
I think if you put the if/else block inside the while block it may be a bit easier to follow plus it will give you a slightly better behavior because you'll be doubling as long as newBufferSize < 1GB and then you'll switch to incremental increases if you cross the 1GB boundary.
PS3, Line 135: public synchronized void reset() {
: }
single line (here and for some of the other functions as well).
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 13:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7955/14/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:
Line 106: if (len >= BUFFER_MAX_SIZE_LIMIT) {
This is also dead code right? We can never hit it.
Might be good to document the preconditions of this function (len > 0, bufferPtr_ >= 0 and len <= BUFFER_MAX_SIZE_LIMIT)
Line 110: if (newBufferPtr == 0) {
Based on your investigation it looks like this is dead code, so let's remove this check.
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;
> It is but we're catching it and trying to release resources. We need to mak
Yes. I didn't realize that failure to reallocate would always throw OOM (for some reason I though it could return 0)
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 17:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/7955/17/fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java
File fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java:
Line 40: || size >= NativeByteArrayOutputStream.BUFFER_MAX_SIZE_LIMIT) {
Our real allocator would not OOM if the size is big, at least not in this place, so let's remove the second check.
Line 60: private void writeAndCheck(NativeByteArrayOutputStream nboas,
I'd prefer to split this up into writeOk() and writeNotOk(). Otherwise, tests may go into the "wrong" code path but still succeed, e.g., because the allocation succeeded and updated the bytesWritten correctly, although we expected the allocation to fail.
Line 78: NativeByteArrayOutputStream nboas =
The first allocation happens here in the c'tor. We need to test that as well.
Alternatively, you can change the nbaos implementation to not allocate in the c'tor (either way is fine with me)
Line 90: nboas = new NativeByteArrayOutputStream(testAllocator);
nit: nbaos
Line 108:
remove extraneous newines
Line 114: testAllocator = new NativeTestAllocator();
Add a helper function for these smaller tests that creates a new allocator, nbaos, array, etc.
http://gerrit.cloudera.org:8080/#/c/7955/17/fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
File fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java:
Line 81: public void testBasicSerialization()
We usually call these TestBasicSerialization (first letter is uppercase)
--
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: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 7:
(13 comments)
Addressed comments other than the test file.
http://gerrit.cloudera.org:8080/#/c/7955/7/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:
Line 114: uint32_t len = static_cast<uint32_t>(nbuffer.buffer_len);
> DCHECK that the buffer_len is smaller than max uint32_t to make it clear th
Done
Line 119: JniUtil::CallJniMethod(catalog_, free_native_byte_buffer_id_, nbuffer);
> Let's check the status of this call and if non-ok emit LOG(ERROR) that n by
Done
http://gerrit.cloudera.org:8080/#/c/7955/7/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
Line 298: if (buffer.buffer_ptr <= 0) return;
> IIRC you said 0 should work. Convert to Preconditions check for < 0.
Done
http://gerrit.cloudera.org:8080/#/c/7955/7/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:
Line 26: * allocations happen on the native side and this class tracks the
> all allocations use native memory and this class tracks the corresponding p
Done
Line 37: * BUFFER_RESIZE_INCREMENTS;
> end sentence with "." (not ";")
Done
Line 77: tryAllocate(initialSize, false);
> For simplicity, let's only allow the default initial size and not a custom
Done
Line 87: throw new IllegalArgumentException("Current size: " + bufferLen_ +
> Isn't this a potential memory leak? Who frees the native memory?
Redid the logic a little. Should cover this.
Line 101: UnsafeUtil.UNSAFE.freeMemory(bufferPtr_);
> This doesn't make sense because bufferPtr_ is already 0. You need to use a
Ouch, good catch.
Line 103: "Failed to (re)allocateMemory() " + len + " bytes");
> also print the value of reAllocate
Done
Line 108: * Write a byte array 'b' of length 'len at offset 'offset'. Resizes the buffer
> Not really accurate. Suggest:
Done
Line 115: throw new IndexOutOfBoundsException(
> Isn't this a potential memory leak? Who frees the native memory?
Done
Line 122: newBufferSize += BUFFER_RESIZE_INCREMENTS;
> Infinite loop if overflow? Same question for L124.
Done
http://gerrit.cloudera.org:8080/#/c/7955/7/fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
File fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java:
Line 62: * Currently that restriction is not enforced by this class and is the responsibility
> Seems easy to enforce this with a flag that gets checked at the beginning o
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7955/10/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
Line 151: if (serializedByteSize >= SERIALIZED_TOPIC_SIZE_WARNING_LIMIT_BYTES) {
> Dimitris and I discussed this warning. If we decide to issue a warning, the
Agreed, that sounds more assertive and meaningful.
--
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 9:
(9 comments)
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
PS9, Line 94: reAllocateMemory
Unsafe.reallocateMemory()
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 them here? Am I missing something?
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.
PS9, Line 78: huge
"huge" doesn't really convey much information here. Maybe a bit more explicit what we're testing (>1GB, < 4GB)? Also, you may want to test multiple sizes to exercise all paths in the reallocation logic (e.g. 500MB, 1GB, 1.5GB, 2GB). You can decide which ones make more sense.
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.
PS9, Line 99:
nit: extra space
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 is larger than 4GB"
PS9, Line 105: never
ha, famous last words :)
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] Use native allocation while building catalog updates
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: [PREVIEW] Use native allocation while building catalog updates
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7955/1/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:
PS1, Line 113: free(buf);
This freaks me out a bit. Any documentation I've read says that you should be using Unsafe.freeMemory() to release memory you allocated using Unsafe. How do we know this is safe?
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#10).
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
IMPALA-5881: Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
This patch also caps the native allocations to not go beyond 4GB due
to Thrift library limitations. (Thrift internally uses uint32_t
datatype to represent the message size which limits the size to
~4.2GB).
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
A fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
10 files changed, 485 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/10
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 9:
(1 comment)
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 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 from ByteArrayOutputStream. Given we mimic it, someone might expe
I get that, but you're not extending the ByteArrayOutputStream class, you're extending OutputStream. So, either change that or remove anything that is not defined in the parent class.
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: [PREVIEW] Use native allocation while building catalog updates
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7955/3/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
PS3, Line 142: This method serializes
: * the output of CatalogService#getCatallogObjects() into native memory allocated
: * using a custom TNativeSerializer and returns the corresponding TNativeByteBuffer
: * serialized into bytes.
> Alternatively: "Uses a custom serializer (TNativeSerializer) to serialize t
Sounds better.
http://gerrit.cloudera.org:8080/#/c/7955/3/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:
PS3, Line 36: 512MB
> update value
Removed them, might become stale again if we update.
Line 54: // Limit on the size to which the underlying buffer can grow
> Comment why that limit exists
Done
PS3, Line 103: off
> offset
Done
PS3, Line 115: if (bufferLen_ >= BUFFER_DOUBLING_RESIZE_LIMIT) {
: while (newBufferSize < bytesWritten_ + len) {
: newBufferSize += BUFFER_RESIZE_INCREMENTS;
: }
: } else {
: while (newBufferSize < bytesWritten_ + len) {
: newBufferSize <<= 1;
: }
: }
> I think if you put the if/else block inside the while block it may be a bit
yea, thats better.
PS3, Line 135: public synchronized void reset() {
: }
> single line (here and for some of the other functions as well).
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 7:
(15 comments)
Still thinking about the warning/logging.
http://gerrit.cloudera.org:8080/#/c/7955/1/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:
PS1, Line 106: TNativeByteBuffer nbuffer;
> makes sense. Done.
Another possible issue is that we run out of memory when constructing the thrift objects, that would throw and be caught inside DeserializeThriftMsg() and turned into a Status. That should be a recoverable condition (hopefully we have enough mem next time).
http://gerrit.cloudera.org:8080/#/c/7955/7/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:
Line 114: uint32_t len = static_cast<uint32_t>(nbuffer.buffer_len);
DCHECK that the buffer_len is smaller than max uint32_t to make it clear that the FE must have enforced this
Line 119: JniUtil::CallJniMethod(catalog_, free_native_byte_buffer_id_, nbuffer);
Let's check the status of this call and if non-ok emit LOG(ERROR) that n bytes have been leaked.
http://gerrit.cloudera.org:8080/#/c/7955/7/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
Line 298: if (buffer.buffer_ptr <= 0) return;
IIRC you said 0 should work. Convert to Preconditions check for < 0.
http://gerrit.cloudera.org:8080/#/c/7955/7/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:
Line 26: * allocations happen on the native side and this class tracks the
all allocations use native memory and this class tracks the corresponding pointer and size.
Line 37: * BUFFER_RESIZE_INCREMENTS;
end sentence with "." (not ";")
Line 77: tryAllocate(initialSize, false);
For simplicity, let's only allow the default initial size and not a custom one.
Line 87: throw new IllegalArgumentException("Current size: " + bufferLen_ +
Isn't this a potential memory leak? Who frees the native memory?
Line 101: UnsafeUtil.UNSAFE.freeMemory(bufferPtr_);
This doesn't make sense because bufferPtr_ is already 0. You need to use a temp variable like newBufferPtr.
Line 103: "Failed to (re)allocateMemory() " + len + " bytes");
also print the value of reAllocate
Line 108: * Write a byte array 'b' of length 'len at offset 'offset'. Resizes the buffer
Not really accurate. Suggest:
Writes 'len' bytes of 'b' starting at 'offset' into the stream....
Line 115: throw new IndexOutOfBoundsException(
Isn't this a potential memory leak? Who frees the native memory?
Line 122: newBufferSize += BUFFER_RESIZE_INCREMENTS;
Infinite loop if overflow? Same question for L124.
Maybe it's better to move the BUFFER_MAX_SIZE_LIMIT check in here.
http://gerrit.cloudera.org:8080/#/c/7955/7/fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
File fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java:
Line 62: * Currently that restriction is not enforced by this class and is the responsibility
Seems easy to enforce this with a flag that gets checked at the beginning of this function and set in a finally block.
http://gerrit.cloudera.org:8080/#/c/7955/7/fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
File fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java:
Line 39: public class TNativeSerializerTest {
It's so critical to get this right, I think it makes sense to hide the native allocations behind an interface so we can mock the allocator. For example, I'm thinking we would create a mock allocator that keeps track of the number of bytes allocated and freed. Then we force the allocator to return 0 or fail in various calls, so we can test that the freed is always equal to the allocated for all error/exception scenarios.
I think we need to test all exceptions that can be thrown from the nbaos, and check that we called free correctly.
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7955/10/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
Line 151: if (serializedByteSize >= SERIALIZED_TOPIC_SIZE_WARNING_LIMIT_BYTES) {
Dimitris and I discussed this warning. If we decide to issue a warning, then I think we should warn when close to the limit, e.g. at 3GB. The warning message should pretty-print the number of bytes and also print the hard limit. Also the message should be more assertive, e.g. "potentially" is vague and should be removed (we know the exact serialized size).
For inspiration, let me propose this msg:
Serialized size of catalog update approaching hard limit. Current size: 3GB. Hard limit: 4GB. Catalog updates that exceed the limit will be ignored and will result in stale metadata on coordinator nodes.
--
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#5).
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
IMPALA-5881: Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
This patch also caps the native allocations to not go beyond 4GB due
to Thrift library limitations. (Thrift internally uses uint32_t
datatype to represent the message size which limits the size to
~4.2GB).
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
A fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
10 files changed, 439 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/5
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#13).
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
IMPALA-5881: Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
This patch also caps the native allocations to not go beyond 4GB due
to Thrift library limitations. (Thrift internally uses uint32_t
datatype to represent the message size which limits the size to
~4.2GB).
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeAllocator.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
A fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java
A fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
12 files changed, 648 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/13
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#8).
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
IMPALA-5881: Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
This patch also caps the native allocations to not go beyond 4GB due
to Thrift library limitations. (Thrift internally uses uint32_t
datatype to represent the message size which limits the size to
~4.2GB).
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
A fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
10 files changed, 502 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/8
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 14:
(2 comments)
I'll send all the addressed changes together in a single PS. Just flushing out comments.
http://gerrit.cloudera.org:8080/#/c/7955/14/fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
File fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java:
Line 106: if (len >= BUFFER_MAX_SIZE_LIMIT) {
> This is also dead code right? We can never hit it.
Done
Line 110: if (newBufferPtr == 0) {
> Based on your investigation it looks like this is dead code, so let's remov
I think it is better to retain the check, just in case the JVM behavior changes :)
--
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: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#17).
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
IMPALA-5881: Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
This patch also caps the native allocations to not go beyond 4GB due
to Thrift library limitations. (Thrift internally uses uint32_t
datatype to represent the message size which limits the size to
~4.2GB).
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeAllocator.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
A fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java
A fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
12 files changed, 716 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/17
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
Patch Set 6:
(2 comments)
Fixed the tests. Figured out that the problem was with thrift doing an initial copy using getBytes() which causes problems. Worked around it by splitting the test struct into 8 strings (instead of 2). Now I can repro all scenarios in unit tests.
http://gerrit.cloudera.org:8080/#/c/7955/1/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:
PS1, Line 106: TNativeByteBuffer nbuffer;
> I'm not really sure what error scenario you are worried about. We allocate
makes sense. Done.
http://gerrit.cloudera.org:8080/#/c/7955/4/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
Line 153: LOG.warn("Detected a potentially large catalog update. Serialized size: "
> I'm not opposed to adding warnings, just concerned that users will worry ab
Hmm, my thinking was that we can use this for postmortem in case of crashes. Let me know your thoughts. I'm fine either way.
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm 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)
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
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
Line 99: * - bufferPtr_>=0
use consistent spacing, i.e. bufferPtr_ >= 0
Line 102: Preconditions.checkState(bufferPtr_ >= 0);
remove (documenting in comment is better here)
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
Line 39: // Throws an OOM on every alternate call to reallocate().
on every 2nd call
Line 51: allocatedBytes_ = 0;
single line
Line 87: while (testAllocator_.getAllocationFailures() < 10) {
I still don't think this is a great approach because it does not reflect real-world usage of the NBAOS.
I think it's worth exhaustively testing all states (there are not that many). You could do something like this instead:
// Test first allocation failure
testAllocator_.setFailRealloc(true);
try {
Nbaos n = new Nbaos(testAllocator_);
} catch () {
// Fill in
}
// Test allocating failure from all possible states, allocating 64M at a time.
int reallocs = 1;
byte[] b = new byte[64 * 1024 * 1024 * 1024];
while (true) {
TestAllocator alloc = new TestAllocator();
Nbaos n = new Nbaos(alloc);
while (alloc.getReallocs() == reallocs) {
n.write(b, 0, len);
}
alloc.setFailRealloc(true);
writeAndCheck();
reallocs = alloc.getReallocs();
// If nbaos is at max capacity break out of loop
}
Line 91: writeAndCheck(b, 1, 0);
These should be tested from a fresh Nbaos and not from the one that is already 'polluted' from previous 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"
Line 119: Assert.assertTrue(nativeSerializer.getSerializedBytesSize() >= 3584 * 1024 * 1024);
Isn't there an assertGe() or something like that? Also print a message with the actual size for debugging
Line 137: e.printStackTrace();
remove
Line 166: testBasicSerialization(factory);
Let's make these separate top-level tests, if's fine to repeat the loop over PROTOCOLS_TO_TEST.
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#16).
Change subject: IMPALA-5881: Use native allocation while building catalog updates
......................................................................
IMPALA-5881: Use native allocation while building catalog updates
This patch moves the allocation of thrift structures for serializing
the output of GetAllCatalogObjects() call into the native side. This
is done to prevent the Catalog from hitting JVM array limitations
(2GB maximum size) at scale. Additionally this patch also extends the
--compact_catalog_top=true to apply TCompactProtocol for the above
serialization to reduce the in-memory footprint.
This patch also caps the native allocations to not go beyond 4GB due
to Thrift library limitations. (Thrift internally uses uint32_t
datatype to represent the message size which limits the size to
~4.2GB).
Testing: Passed ASAN + HDFS core and DEBUG + HDFS exhaustive.
Deployed the patch on a 16 node cluster and tested it on a
Catalog-update topic of 3.5GB (uncompressed) or 780MB (compressed).
Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
---
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/jni-thrift-util.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/thrift/NativeAllocator.java
A fe/src/main/java/org/apache/impala/thrift/NativeByteArrayOutputStream.java
A fe/src/main/java/org/apache/impala/thrift/TNativeSerializer.java
A fe/src/test/java/org/apache/impala/thrift/NativeByteArrayOutputStreamTest.java
A fe/src/test/java/org/apache/impala/thrift/TNativeSerializerTest.java
12 files changed, 672 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/7955/16
--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>