Tim Armstrong has posted comments on this change. Change subject: IMPALA-4674: Part 1: port BufferedTupleStream to BufferPool ......................................................................
Patch Set 11: (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 val We don't check at the exit points of all functions so I wanted to at least check at the entry to some of these functions before they start changing state. Line 270: RETURN_IF_ERROR(PinPageIfNeeded(write_page_, pinned_)); > wouldn't it be better to move this to AdvanceWritePage()? The other caller Done Line 299: // May need to pin the new page for both reading and writing. > Add: "See ExpectedPinCount()." Done Line 374: pinned_ || buffer_pool_client_->reservation()->IncreaseReservationToFit(page_len_); > let's add a comment, something like: Done 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 P Yeah, I think we should really remove it: IMPALA-2758 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 Done PS11, Line 518: write iterator is > iterators are Done -- 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
