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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
