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