Tim Armstrong has posted comments on this change. Change subject: IMPALA-3201: reservation implementation for new buffer pool ......................................................................
Patch Set 8: (25 comments) 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 Done 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? Seems more useful for debugging in the case when the reservation isn't granted. Otherwise we don't have any info in the profile about the reservation it tried to grab. We don't lose any info by capturing it. PS8, Line 35: Counter > these peak ones look like they're actually HighWaterMarkCounters. Will change to the subclass. HighWaterMarkCounter doesn't report the current value in the profile. I agree its redundant but I don't see an easy fix without significant changes to the runtime profiles to have multi-valued counters. 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. Done 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 ne Clarified that it was inclusive of resources that are currently used. I think that was the main ambiguity, right? Line 55: /// child_reservations + used_reservation <= reservation. > maybe just introduce "unused_reservation" here so this can be equality. Done Line 56: /// > let's have a quick explanation of thread-safety rules Done PS8, Line 58: can > will Done PS8, Line 58: integration > (and MemTracker) Done PS8, Line 62: included in memory limit checks > this could use some updating/clarification Tried to clarify this. PS8, Line 69: assume > require that Done PS8, Line 71: assume > require Done Line 73: /// ancestors is also linked to a MemTracker "B", then "B" must be an ancestor of "A". I made this stricter - no gaps in the hierarchy. We can just add in ReservationTrackers at the fragment level to make this work. PS8, Line 79: . > I know you want to keep ReservationTracker for resources in general (as opp Denominating everything in bytes should go a long way to eliminating that confusion. I'm fine with using bytes, someone else can probably deal with the problem down the road if this turns out the be the right abstraction for some other resource. PS8, Line 83: counters_ > the public interface shouldn't really refer to private stuff. just explain Done PS8, Line 85: MemTracker* mem_tracker > given our requirement that the top most link is the one that enforces limit Yes, it will be typically NULL since we need to enforce the limit one level down at the query mem trackers. I actually removed this parameter so that it is always NULL since we don't need it. PS8, Line 103: amount > also confusing to use without knowing units Went through and changed 'amount' to 'bytes' where applicable and reworded if appropriate. PS8, Line 109: 'amount' is available to use > there is 'amount' of unused reservation. (for consistency of terminology). Done PS8, Line 123: back to the reservation > this can be confusing because it can be interpretted as incrementing reserv Used that but said "release" instead of "free" to be consistent with the method name. PS8, Line 127: memory > here you say memory (which I'm fine with, but let's be consistent one way o Done PS8, Line 134: or > also hard to parse whether the second condition is included or not in "unus Done Line 134: /// Returns the amount of the reservation not used or given to childrens' reservations. > at this tracker Done Line 148: /// Initializes 'counters_', storing the counters in 'profile'. > Wasn't sure what this meant without reading the code. How about: Updated to reflect the dummy profile change. 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 h Done. It's a little tricky since there is no RuntimeProfile at the query level. MemTracker has a local_counter_ field to work around this. To get around this, I created a DummyProfile class that can be used to create a dummy profile to hold the counters. PS8, Line 77: COUNTER_ADD_NOT_NULL > this doesn't look needed 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: 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
