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 18: (1 comment) http://gerrit.cloudera.org:8080/#/c/8707/18/be/src/runtime/io/disk-io-mgr.h File be/src/runtime/io/disk-io-mgr.h: http://gerrit.cloudera.org:8080/#/c/8707/18/be/src/runtime/io/disk-io-mgr.h@271 PS18, Line 271: /// If 'needs_buffers' is set to true, the caller must then call AddBuffersToRange() to > Can we merge "the caller must then call..." work into this function and rem I just realised that I never responded to this comment (it was a good one and required some thought). I think at all current call sites we could combine the two functions, which might be simpler in some ways. My thought though was that, in general, callers shouldn't have to reserve the memory before knowing how much memory the scan range needs (e.g. if it's a cached range, it may not require buffers). I.e. with this interface, callers can do the following: Call StartScanRange() If the range needs buffers, figure out how much memory to reserve and where to get it from. As opposed to: Reserve the memory conservatively Call StartScanRange() I can see arguments for either merging the functions and simplifying the API or leaving it more general. GetNextRange() has a similar pattern, but there you don't even know what range you'll be scanning until you call the function, so callers would have a hard time knowing what a reasonable amount to reserve is. -- 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: 18 Gerrit-Owner: Tim Armstrong <tarmstr...@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: Wed, 31 Jan 2018 16:38:49 +0000 Gerrit-HasComments: Yes
