Dan Hecht has posted comments on this change. Change subject: IMPALA-5239: 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 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 Transfer? and what does this one mean? 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 reservation? what are the other cases used for? I guess the first will be used when an exec-node is done (and is similar to DecreaseReservation())? And the third will eventually be used when transferring between non-overlapping exec-nodes? 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"? PS3, Line 199: decrease how about reservation_decrease to be clear this is opposite TryConsumeFromMemTracker and the fact this is is bookkeeping for reservation decrease. -- 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-HasComments: Yes
