Dan Hecht has posted comments on this change. Change subject: IMPALA-3201: reservation implementation for new buffer pool ......................................................................
Patch Set 8: (26 comments) Focused on the public interface. http://gerrit.cloudera.org:8080/#/c/3993/8/be/src/bufferpool/buffer-pool.h File be/src/bufferpool/buffer-pool.h: PS8, Line 266: s given the way we use this term in reservation-tracker, seems this should be singular. http://gerrit.cloudera.org:8080/#/c/3993/8/be/src/bufferpool/reservation-tracker-counters.h File be/src/bufferpool/reservation-tracker-counters.h: Line 28: /// This is set even if the reservation was not granted. why is that? PS8, Line 31: in bytes here you do use units, while in ReservationTracker you try to stay resource agnostic (I prefer this where units are specified -- see comments in reservation-tracker.h as well). PS8, Line 35: Counter these peak ones look like they're actually HighWaterMarkCounters. Also, why have these peak counters, rather than just having the 'reservation' counter by a high water mark counter that keeps track of this peak? http://gerrit.cloudera.org:8080/#/c/3993/8/be/src/bufferpool/reservation-tracker.h File be/src/bufferpool/reservation-tracker.h: PS8, Line 40: and accounted against the tracker : /// directly this is kind of vague. PS8, Line 41: its the child's to disambiguate. Line 44: /// reservation (and perhaps increase reservations all the way to the root of the tree). given the various interpretations of "reservation" that we discussed, we need to spell out the various terms more carefully. Line 55: /// child_reservations + used_reservation <= reservation. maybe just introduce "unused_reservation" here so this can be equality. Line 56: /// let's have a quick explanation of thread-safety rules PS8, Line 58: can will PS8, Line 58: integration (and MemTracker) PS8, Line 62: included in memory limit checks this could use some updating/clarification PS8, Line 69: assume require that PS8, Line 71: assume require PS8, Line 79: . I know you want to keep ReservationTracker for resources in general (as opposed to memory specific), but we need some way to specify the units (readers won't know if it's bytes or buffers, etc) PS8, Line 83: counters_ the public interface shouldn't really refer to private stuff. just explain what it adds to the profile. PS8, Line 85: MemTracker* mem_tracker given our requirement that the top most link is the one that enforces limit, is this allowed to be non-NULL? PS8, Line 103: amount also confusing to use without knowing units PS8, Line 109: 'amount' is available to use there is 'amount' of unused reservation. (for consistency of terminology). PS8, Line 123: back to the reservation this can be confusing because it can be interpretted as incrementing reservation by 'account' (if we had used the alternate terminology). How about: Free up 'amount' of previously allocated resources. The used reservation is decreased by 'amount'. PS8, Line 127: memory here you say memory (which I'm fine with, but let's be consistent one way or another). Line 134: /// Returns the amount of the reservation not used or given to childrens' reservations. at this tracker PS8, Line 134: or also hard to parse whether the second condition is included or not in "unused reservation". how about: ... not used nor given to children's reservations. Line 148: /// Initializes 'counters_', storing the counters in 'profile'. Wasn't sure what this meant without reading the code. How about: If 'profile' is not NULL, adds this tracker's set of runtime profile counters to 'profile'. http://gerrit.cloudera.org:8080/#/c/3993/8/be/src/util/runtime-profile-counters.h File be/src/util/runtime-profile-counters.h: PS8, Line 57: COUNTER_SET_IF_NOT_NULL rather than adding these (and having to know to use them), why not always have valid runtime counters? PS8, Line 77: COUNTER_ADD_NOT_NULL this doesn't look needed -- 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: 8 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
