Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201707131659.v6DGxpDU029097@ip-10-146-233-104.ec2.internal>
Date: Thu, 13 Jul 2017 16:59:50 +0000
From: "Bikramjeet Vig (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Matthew Jacobs <mj@cloudera.com>, Tim Armstrong <tarmstrong@cloudera.com>
Reply-To: bikramjeet.vig@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5520=3A_TopN_node_periodically_reclaims_old_allocations=0A?=
In-Reply-To: <gerrit.1499812062000.I968f57f0ff2905bd581908bc5c5ee486b31e6aa8@gerrit.cloudera.org>
References: <gerrit.1499812062000.I968f57f0ff2905bd581908bc5c5ee486b31e6aa8@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: Thu, 13 Jul 2017 16:59:57 -0000

Bikramjeet Vig has posted comments on this change.

Change subject: IMPALA-5520: TopN node periodically reclaims old allocations
......................................................................


Patch Set 1:

(19 comments)

http://gerrit.cloudera.org:8080/#/c/7400/1//COMMIT_MSG
Commit Message:

PS1, Line 9: tuple
> in a tuple pool which is held longer than necessary, resulting in unnecessa
Done


PS1, Line 11: With this commit TopN node
> commit, the TopN node
Done


PS1, Line 12: its
> stored in the priority queue
Done


PS1, Line 12: This is done every time the
            : number of old rows (removed from the priority queue it uses) is more
            : than twice the row limit (N in TopN)
> This is done when the number of rows removed from the priority queue is mor
Done


PS1, Line 24: general
> expected general case
Done


PS1, Line 27: data
> example worst case: data stored...
Done


PS1, Line 39: performance
> query runtimes
Done


PS1, Line 39: More extensive perf testing will be
            : done in the future to recognize pathological cases
> Not necessarily pathological cases, more that we're gonna do cluster testin
Done


http://gerrit.cloudera.org:8080/#/c/7400/1/be/src/exec/topn-node-ir.cc
File be/src/exec/topn-node-ir.cc:

PS1, Line 28: limit_
> why isn't this (limit_ + offset_), since that's the size of the PQ?
Done


Line 61: void TopNNode::ReclaimTuplePool() {
> I don't think this needs to be in the *-ir.cc - the stuff in here gets reco
Done


Line 62:   auto temp_pool = new MemPool(mem_tracker());
> It would be good to use unique_ptr here just so we don't accidentally creat
Done


Line 63:   auto temp_pq = new std::priority_queue<Tuple*, std::vector<Tuple*>,
> Rebuilding the priority queue like this seems unnecessary. It's a std::vect
Done


PS1, Line 69: Allocate
> We're trying to move to using TryAllocate() instead and failing synchronous
Done


http://gerrit.cloudera.org:8080/#/c/7400/1/be/src/exec/topn-node.cc
File be/src/exec/topn-node.cc:

PS1, Line 88: NumOfTimesTuplePoolReclaimed
> TuplePoolReclamations is more concise
Done


PS1, Line 88:   
> indentation
Done


http://gerrit.cloudera.org:8080/#/c/7400/1/be/src/exec/topn-node.h
File be/src/exec/topn-node.h:

PS1, Line 71: Create new tuple pool with only the elements inside the priority queue and release
            :   // memory from the old tuple pool
> Re-materialize the elements in the priority queue into a new tuple pool, an
Done


PS1, Line 115: poped
> popped has two 'p's. Regardless, let's call this rows_to_reclaim_ because n
Done


http://gerrit.cloudera.org:8080/#/c/7400/1/tests/query_test/test_tpch_queries.py
File tests/query_test/test_tpch_queries.py:

Line 67:     return 'tpch'
> newline after this
Done


PS1, Line 75:  \
> nit: remove space to be consistent
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I968f57f0ff2905bd581908bc5c5ee486b31e6aa8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes