impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4023: don't attach buffered tuple streams to batches
Date Thu, 22 Sep 2016 23:56:24 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4023: don't attach buffered tuple streams to batches
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4448/1/be/src/runtime/buffered-tuple-stream-test.cc
File be/src/runtime/buffered-tuple-stream-test.cc:

Line 1185: }
> Add a simple test that checks pinned blocks are attached to a non-NULL batc
Done


http://gerrit.cloudera.org:8080/#/c/4448/1/be/src/runtime/row-batch.h
File be/src/runtime/row-batch.h:

Line 129:   bool ALWAYS_INLINE AtCapacity() {
> The meaning of AtCapacity() has slightly changed and I think it might be a 
We didn't check need_to_return_ before, it was sufficient to check num_rows_ == capacity_.
The DCHECK I removed was redundant with the remaining one.

I think you're right that it's changed slightly, but I think the new behaviour makes more
sense. The cases are:
1. The stream is empty - it doesn't make sense to consider it AtCapacity()
2. The stream has only small buffers < 8MB - in this case I think it's fine to not consider
the batch AtCapacity() until the memory adds up to >= 8MB.
3. The stream has at least one large buffer >= 8MB - the aux memory usage check will cause
this to be AtCapacity().

There's a bigger preexisting problem with the accounting of the attached blocks: the accounting
isn't updated for the purpose of MemTrackers and BufferedBlockMgr reservations, so it's possible
for another exec node to accumulate batches that are accounted against a different node. I'm
going to need to fix that for the BufferPool changes, but I'm punting on it for now. This
change will make that change simpler.


-- 
To view, visit http://gerrit.cloudera.org:8080/4448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6471422f86ce71e4c6ab277a276000051bc2e8ff
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message