Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201705101747.v4AHlpP7027806@ip-10-146-233-104.ec2.internal>
Date: Wed, 10 May 2017 17:47:51 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Dan Hecht <dhecht@cloudera.com>
Reply-To: tarmstrong@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5238=3A_transfer_reservations_between_trackers=0A?=
In-Reply-To: <gerrit.1492733518000.I21f008abaf1aa4fcd2d854769a603b97589af3b3@gerrit.cloudera.org>
References: <gerrit.1492733518000.I21f008abaf1aa4fcd2d854769a603b97589af3b3@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, 10 May 2017 17:47:58 -0000

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5238: transfer reservations between trackers
......................................................................


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/6708/3//COMMIT_MSG
Commit Message:

PS3, Line 7: 5239
> wrong number
Done


http://gerrit.cloudera.org:8080/#/c/6708/3/be/src/runtime/bufferpool/reservation-tracker.cc
File be/src/runtime/bufferpool/reservation-tracker.cc:

PS3, Line 204: void ReservationTracker::DecreaseReservation(int64_t bytes) {
> does this interface still make sense? i.e. do we need both this and Transfe
We still need this internally to implement Close() and TransferReservationTo(). 

This does do something distinct from either - it releases reservation on all ancestors without closing it. TransferReservationTo() could only be used to release reservation up to the root.

I don't that is really necessary though for now, so I removed the public interface and updated tests to use alternative APIs.


PS3, Line 253: * 'other' is a descendent of 'this'. 'path_to_root' is empty because 'this' is the
             :   //    lowest common ancestor. To transfer, we increase the reservation on the trackers
             :   //    under 'this', down to 'other'.
> isn't this the only case we need for the distribution of initial reservatio
The third case is the one we need to distribute reservation - the initial reservation can't be in the query root ReservationTracker since then there's no way to prevent it being used for non-initial reservations. It needs to be in a separate "initial reservation" tracker that is a child of the query tracker (e.g. the distribution would be the same as "aunt"->"child" in the unit test).

Now that I think about it, I think the underlying problem is the same as the scenario you mention - some ExecNodes won't claim their reservation until after the query has started executing, which means that in the meantime the ExecNodes initial reservation needs to be stored somewhere, then transferred.


http://gerrit.cloudera.org:8080/#/c/6708/3/be/src/runtime/bufferpool/reservation-tracker.h
File be/src/runtime/bufferpool/reservation-tracker.h:

PS3, Line 197: Release memory on linked MemTracker
> not sure what that means. should it say "to linked MemTracker"?
Reworded


PS3, Line 199: decrease
> how about reservation_decrease to be clear this is opposite TryConsumeFromM
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I21f008abaf1aa4fcd2d854769a603b97589af3b3
Gerrit-PatchSet: 3
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