Tim Armstrong has posted comments on this change. Change subject: IMPALA-3201: reservation implementation for new buffer pool ......................................................................
Patch Set 3: (7 comments) 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 MemTrac The coupling with MemTracker is optional: if the mem_tracker is NULL, everything works. There's no reason you couldn't use the code to account for some other resource if mem_tracker_ is NULL. I've gone back and forth about this. I don't usually like speculative generality, but there's very little about the code or abstraction that's specific to memory reservations. I think it would be unfortunate if we have a need in the future to track other resource reservations and had to reinvent the wheel. I'm happy to change this. 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-le If an exec node subdivides its allocation using ReservationTrackers below the node-level tracker. I think it naturally occurs there, since you would usually count some usage again the exec node's tracker. PS3, Line 41: consume > to avoid confusion with memtracker consume/release, how about using the ter Done. Missed it on an earlier pass. PS3, Line 51: consumption > usage Done PS3, Line 55: optionally > what cases do we not want to have an associated MemTracker? I noticed the same thing about the invariant. Hopefully the new version is better. I don't think it makes sense to associate the root MemTracker and ReservationTracker right now, since the root MemTracker usage is derived from a metric value rather than maintained in the usual way. Eventually if all the memory is allocated from the buffer pool we could change that (or get rid of the MemTrackers entirely). Also for ReservationTrackers below the node-level ReservationTracker. E.g. if we're allocating temporary trackers to subdivide the reservation. I think it would be inconvenient to allocate many corresponding MemTrackers. PS3, Line 79: InitChildTracker > For InitRootTracker/InitChildTracker(), maybe these should be static method Right, it's already enforced with a DCHECK. The main reason I did it this way is to avoid having to make another heap allocation and decide whether it should return a raw/unique/scoped/whatever pointer. I don't feel strongly, I think it works ok either way, so I could switch it to return a unique_ptr (or something like that). 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 r Done -- 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
