Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201703152359.v2FNxDnU006905@ip-10-146-233-104.ec2.internal>
Date: Wed, 15 Mar 2017 23:59:13 +0000
From: "Dan Hecht (Code Review)" <gerrit@cloudera.org>
To: Tim Armstrong <tarmstrong@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
Reply-To: dhecht@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4674=3A_Part_1=3A_port_BufferedTupleStream_to_BufferPool=0A?=
In-Reply-To: <gerrit.1485543124000.I7bb47a818564ac19a6be4ca88db0578f4ea0b709@gerrit.cloudera.org>
References: <gerrit.1485543124000.I7bb47a818564ac19a6be4ca88db0578f4ea0b709@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Wed, 15 Mar 2017 23:59:22 -0000

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