Dan Hecht has posted comments on this change. Change subject: IMPALA-3201: reservation implementation for new buffer pool ......................................................................
Patch Set 8: (16 comments) Only partially made it through the rest of the change, still going. But wanted to flush out my response to the reservation-tracker-counters.h thread. http://gerrit.cloudera.org:8080/#/c/3993/8/be/src/bufferpool/reservation-tracker-counters.h File be/src/bufferpool/reservation-tracker-counters.h: PS8, Line 35: Counter > There's an UpdateReservation() method I added in the last patchset that doe Right it's similar to UpdateReservation(), but encapsulated in this class so that ReservatIonTracker() doesn't have to know about this oddity of the profile counters. Also, I was envisioning then that this class would expose accessors: reservation(), used_reservation(), reservation_limit(), so it wouldn't really be verbose. This came up because when starting to read the code I had the question about whether the state in ReservationTracker and the state in these counters were always in sync. The DCHECKs answered those questions (so leave them if you keep the state separate), but then was thinking it would be nice to eliminate the redundant state altogether. That is, the motivation isn't to get rid of the DCHECKs but to eliminate the redundancy in state (which makes it easier to understand for the same reason we eliminate the DCHECKs -- there's no state redundancy). I wasn't sure if you were shadowing the sate to avoid the atomic. But note that atomic load/store is unlocked, so is just as fast. So I don't see this as a motivation for mirroring the state. And I don't see it weirder than mirroring the state -- we're still using both both locks and atomic variable techniques one way or the other. I don't feel too strongly about it but just seems to me that eliminating state redundancy (and pushing down what we can't eliminate) is clearest. Why do you feel it's cleaner to have the redundant state? http://gerrit.cloudera.org:8080/#/c/3993/8/be/src/bufferpool/reservation-tracker.h File be/src/bufferpool/reservation-tracker.h: Line 44: /// reservation (and perhaps increase reservations all the way to the root of the tree). > Clarified that it was inclusive of resources that are currently used. I thi yes PS8, Line 79: . > Denominating everything in bytes should go a long way to eliminating that c Thanks. PS8, Line 85: MemTracker* mem_tracker > Yes, it will be typically NULL since we need to enforce the limit one level Great. http://gerrit.cloudera.org:8080/#/c/3993/9/be/src/bufferpool/reservation-tracker.h File be/src/bufferpool/reservation-tracker.h: Line 121: void AllocateFrom(int64_t amount); Still not clear to me if we need both variants. But we can leave for now and I'll look again once the client code comes in. PS9, Line 130: cu before calling this method (i.e. clarify it's not a global invariant or post condition. it's a precondition). PS9, Line 134: before calling this method. PS9, Line 137: en. memory (now that we're being a bit less abstract) PS9, Line 169: es what does this do if it's false? PS9, Line 179: For the topmost link, return false if ... Line 196: /// All non-NULL if 'initialized_' is true and a RuntimeProfile was provided during profile counters PS9, Line 200: es not c profile counters PS9, Line 217: urrent reservation. 'reservation_' <= 'reservation_limit_ update Line 231 with this simplification, is this worth it, or should we just do parent_->mem-tracker_? http://gerrit.cloudera.org:8080/#/c/3993/9/be/src/util/runtime-profile-counters.h File be/src/util/runtime-profile-counters.h: Line 499 why is this header the right place (as opposed to runtime-profile.h)? http://gerrit.cloudera.org:8080/#/c/3993/9/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: Line 27: #include "util/spinlock.h" why? i only see indirect references to ObjectPool. -- 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
