Tim Armstrong has posted comments on this change. Change subject: IMPALA-3201: reservation implementation for new buffer pool ......................................................................
Patch Set 8: (1 comment) 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 > I see. We could hide this detail inside of this class by defining an SetRe There's an UpdateReservation() method I added in the last patchset that does more-or-less what you're suggesting unless there's some subtlety that I'm missing. I could remove the DCHECKs for the counter values - those might be overkill. The main reason I added them initially was because you get some weird behaviour if you have trackers sharing the same profile and therefore the same counters. I already have a DCHECK to check for that when adding so should be less of an issue. I'm ok with changing to use the counters if you prefer, but not sure if it works out cleaner. It feels a little weird to use the atomic operations to read member variables protected by a lock but ultimately it may not matter that much. It might make the code a bit more verbose too (i.e. ->value() everywhere). -- 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
