Dan Hecht has posted comments on this change. (
Change subject: IMPALA-4835: Part 2: Allocate scan range buffers upfront
Patch Set 24:
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-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