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 <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 13 Feb 2018 07:40:35 +0000 Gerrit-HasComments: Yes
