Tim Armstrong has posted comments on this change. Change subject: IMPALA-3201: reservation implementation for new buffer pool ......................................................................
Patch Set 9: (13 comments) http://gerrit.cloudera.org:8080/#/c/3993/9/be/src/bufferpool/reservation-tracker-counters.h File be/src/bufferpool/reservation-tracker-counters.h: Line 33: /// The tracker's current reservation in bytes. As discussed, I removed the "current" counters since it's not particularly useful to report in profiles. http://gerrit.cloudera.org:8080/#/c/3993/9/be/src/bufferpool/reservation-tracker.h File be/src/bufferpool/reservation-tracker.h: Line 121: bool IncreaseReservation(int64_t bytes); > Still not clear to me if we need both variants. But we can leave for now a IncreaseReservation() seems like the better primitive since you can implement everything in terms of it. It's convenient for tests too. We could potentially make it a private method for testing only. IncreaseReservationToFit() is relatively easy to implement on top of IncreaseReservation() but has a more complicated contract, I just added it to the interface originally to avoid duplicating the calculation all over the place. I thought about making it an external utility method to compute the required reservation increase, but that got verbose. PS9, Line 130: '. > before calling this method (i.e. clarify it's not a global invariant or pos Done PS9, Line 134: . > before calling this method. Done PS9, Line 137: resources > memory (now that we're being a bit less abstract) Done PS9, Line 169: true > what does this do if it's false? Done PS9, Line 179: Return > For the topmost link, return false if ... Done Line 196: /// Increase or decrease 'used_reservation_' and update counters accordingly. > profile counters Done PS9, Line 200: counters > profile counters Done PS9, Line 217: If no RuntimeProfile was provided, all counters are NULL. > update Done Line 231: MemTracker* parent_mem_tracker_; > with this simplification, is this worth it, or should we just do parent_->m Removed it. The checks get a little more complicated (have to check if both parent and parent->mem_tracker are NULL) but it works out ok. 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)? I didn't want to put in in runtime-profile.h since it would be polluting a widely-included header with a rarely used class. I moved to dummy-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 "common/object-pool.h" > why? i only see indirect references to ObjectPool. Needed for DummyProfile - missed moving it to the other file. Now in dummy-runtime-profile.h -- 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: 9 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
