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