Dan Hecht has posted comments on this change. Change subject: IMPALA-3201: reservation implementation for new buffer pool ......................................................................
Patch Set 3: (7 comments) Some initial comments. I think if you clean up some of the terminology it will be easier to follow and do the rest of the review. http://gerrit.cloudera.org:8080/#/c/3993/3/be/src/bufferpool/reservation-tracker.h File be/src/bufferpool/reservation-tracker.h: PS3, Line 32: e.g. bytes of memory is this really "e.g." and "resources" given the tight coupling with MemTracker? Maybe we should just say "memory reservations" to be clearer about what this is. Line 39: /// directly, or distributed to children of the tracker for its own reservation. are there cases where a client would use reservation directly from a non-leaf node in the hierarchy? PS3, Line 41: consume to avoid confusion with memtracker consume/release, how about using the term "use" (to go along with IncreaseUsage()/DecreaseUsage())? PS3, Line 51: consumption usage PS3, Line 55: optionally what cases do we not want to have an associated MemTracker? and it seems the last two can be specified as a more precise single invariant (though I'm not sure since I don't quite follow the last one). PS3, Line 79: InitChildTracker For InitRootTracker/InitChildTracker(), maybe these should be static methods that also construct the ReservationTracker() and then make the constructor private. That way it's clear that each reservation tracker should be initialized to exactly one of these "personalities" (I assume that's the case). PS3, Line 83: The reservation must be completely : /// released before calling Close(). This seems to contradict the previous sentence. Oh, I guess you mean the reservation usage must be returned before calling Close()? -- To view, visit http://gerrit.cloudera.org:8080/3993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
