impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4674: Part 1: port BufferedTupleStream to BufferPool
Date Wed, 15 Mar 2017 23:59:13 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message