Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8707 )

Change subject: IMPALA-4835: Part 2: Allocate scan range buffers upfront
......................................................................


Patch Set 24:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8707/24/be/src/runtime/io/disk-io-mgr.h
File be/src/runtime/io/disk-io-mgr.h:

http://gerrit.cloudera.org:8080/#/c/8707/24/be/src/runtime/io/disk-io-mgr.h@144
PS24, Line 144: recycle
> is that the same as putting them back on the queue you talk about in the ne
Yes it is. Reworded.


http://gerrit.cloudera.org:8080/#/c/8707/24/be/src/runtime/io/disk-io-mgr.h@301
PS24, Line 301:   /// and the state of 'range' is unmodified so that allocation 
can be retried.
> so does this schedule the scan range (given that the previous two methods l
It only makes sense to call after *needs_buffers because the calculation for # 
buffers to allocate only makes sense for that. I think it would work otherwise, 
but doesn't make sense. Updated comment to explain the cases when it should be 
called.


http://gerrit.cloudera.org:8080/#/c/8707/24/be/src/runtime/io/disk-io-mgr.h@308
PS24, Line 308:       int64_t max_bytes);
> If we aren't able to reduce the number of ExternalBufferTag cases, I'm not
It felt awkward to force the caller to specify the amount of memory to use for 
the range before it knew how much memory the ScanRange actually needed. With 
GetNextRange() in particular, the caller doesn't even know anything about range 
it will be processing so it can't make a smart decision about how much memory 
to use. We don't do this now so we could combine them, but it didn't feel 
particularly natural to me.

I also liked that the buffer allocation was an explicit method call. Bundling 
it into another IoMgr method and controlling it by an argument seemed like it 
hid the memory allocation a bit.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia243bf8b62feeea602b122e0503ea4ba7d3ee70f
Gerrit-Change-Number: 8707
Gerrit-PatchSet: 24
Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Feb 2018 07:40:35 +0000
Gerrit-HasComments: Yes

Reply via email to