Dan Hecht has posted comments on this change. Change subject: IMPALA-4674: Part 1: port BufferedTupleStream to BufferPool ......................................................................
Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/5811/5/be/src/runtime/buffered-tuple-stream-v2.cc File be/src/runtime/buffered-tuple-stream-v2.cc: Line 256: // actually fail once we have variable-length pages. > I just kept this logic from the old BufferedTupleStream. How would the stream use more reservation after UnpinStream()? Wouldn't we spend a reservation on the read and write pages both when the stream is pinned and then unpinned? I need to read the code through some more, but it seems to that a fair amount of complexity comes from sharing the reservation when they overlap. If we do end up sticking with this scheme, how about verifying the pin count is exactly 1 for the read/write page(s)? http://gerrit.cloudera.org:8080/#/c/5811/6/be/src/runtime/buffered-tuple-stream-v2.cc File be/src/runtime/buffered-tuple-stream-v2.cc: PS6, Line 112: DCHECK_LE(read_ptr_, read_end_ptr); would be nice to write this and line 106 the same to make it a little faster to read Line 252: if (!read_page_->is_pinned()) { why does this not have to check if read and write page are the same? Line 276: if (!pinned_ && write_page_ != &pages_.front()) { this could use a comment: // pages_.front() will become the read_page_. Line 332: && (&page == write_page_ || (read_write_ && &page == &*read_page_))) { why is this right to unpin the read_page_ if !read_write_? http://gerrit.cloudera.org:8080/#/c/5811/6/be/src/runtime/buffered-tuple-stream-v2.h File be/src/runtime/buffered-tuple-stream-v2.h: PS6, Line 517: CalcNumPinned this looks dead -- To view, visit http://gerrit.cloudera.org:8080/5811 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7bb47a818564ac19a6be4ca88db0578f4ea0b709 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
