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: (3 comments) A few initial questions. 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 next paragraph? If so, maybe say re-enqueue? 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 left the range unscheduled when '*needs_buffers'? is it legal to call this only after '*needs_buffers' is returned true, or can it be called regardless? 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 sure I follow the benefit of this interface. Why not just pass the reservation through to AddScanRanges()/StartScanRange(), since the io_mgr seems to still be managing the buffer lifetimes anyway? -- 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 01:37:11 +0000 Gerrit-HasComments: Yes
