Dan Hecht has posted comments on this change. Change subject: IMPALA-4674: Part 1: port BufferedTupleStream to BufferPool ......................................................................
Patch Set 11: Code-Review+2 (7 comments) http://gerrit.cloudera.org:8080/#/c/5811/11/be/src/runtime/buffered-tuple-stream-v2.cc File be/src/runtime/buffered-tuple-stream-v2.cc: Line 176: CHECK_CONSISTENCY(); maybe move these CHECK_CONSISTECY() to the end of the functions so they validate the resulting state, but feel free to ignore. Line 270: RETURN_IF_ERROR(PinPageIfNeeded(write_page_, pinned_)); wouldn't it be better to move this to AdvanceWritePage()? The other callers handle it for their specific case (i.e. by calling PrepareForReadInternal(), or not needing it), so this is really just for when we're adding a new write page to an existing stream, no? Line 299: // May need to pin the new page for both reading and writing. Add: "See ExpectedPinCount()." since that's where we really document how we manage reservations/pins. Line 374: pinned_ || buffer_pool_client_->reservation()->IncreaseReservationToFit(page_len_); let's add a comment, something like: // If already pinned, no additional pin is needed (see ExpectedPinCount()). Line 443: MemTracker* tracker, scoped_ptr<RowBatch>* batch, bool* got_rows) { you don't have to do it now, but I kinda think this function should be in PHJ. It's really just a composition of buffered-tuple-stream operations, and it's not something that we should be doing generally. http://gerrit.cloudera.org:8080/#/c/5811/11/be/src/runtime/buffered-tuple-stream-v2.h File be/src/runtime/buffered-tuple-stream-v2.h: PS11, Line 498: and to pin it for reading if the stream's current : /// state requires it. see comment in cc file on this function. if you take that suggestion, then update this. PS11, Line 518: write iterator is iterators are -- 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: 11 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
