Dan Hecht 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:

(1 comment)

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@308
PS24, Line 308:       int64_t max_bytes);
> It felt awkward to force the caller to specify the amount of memory to use
But given that a scanner will be scanning multiple scan ranges, don't we have 
to assume the worse case when assigning reservations? So what does it matter 
how big the current scan range is?

I can see the argument for not hiding the memory allocation. But on the flip 
side, that's kind of the benefit of reservations -- the policy logic is just 
divvying up the reservation and that is inside the scan node and scanner.

Also, a different question relating to how things are organized given we have 
reservations: why do we need to queue the buffers per range rather than just 
allocate them from the buffer pool, given that the reservations should ensure 
they are available. i.e .why isn't it a matter of reimplementing 
TryAllocateNextBufferForRange() to use the assigned reservation to get the 
buffer from the buffer pool?

It's a bit hard to understand how this all comes together with the patches 
divided given that the reservation logic is not a trivial addition. Could you 
generate a CR that has part 2&3 combined?



--
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 17:39:33 +0000
Gerrit-HasComments: Yes

Reply via email to