Tim Armstrong has posted comments on this change. Change subject: IMPALA-3201: reservation implementation for new buffer pool ......................................................................
Patch Set 9: (10 comments) http://gerrit.cloudera.org:8080/#/c/3993/10/be/src/bufferpool/reservation-tracker.cc File be/src/bufferpool/reservation-tracker.cc: PS10, Line 100: > Shouldn't we introduce a Reservation-specific error code? Okay to defer fo I was deferring it until I can solve it more holistically in the context of query startup. E.g. if we fail to get the initial reservation I think we want an error message that reports the full reservation it was trying to acquire across all exec nodes. Now that I think about it, maybe I should just remove the initial_reservation argument/concept from the ReservationTracker. Not reason it can't be implemented with InitChildTracker() then IncreaseReservation(). What do you think? Line 143: > This comment is confusing because it sounds like this is a precondition to Done Line 145: return IncreaseReservationInternal(bytes, false, false); > the method comment claims that this unlinks from parent, but parent_ is not Done 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); > My motivation for bringing it up is because it'd be nice to remove the !use It will be useful in setting up initial reservations: we can request the entire initial reservation at the query level, then during Prepare() each exec node can pull down its portion of the reservation. As you mentioned it's also useful (necessary, even) if an exec node wants to subdivide its initial reservation. With the current primitives you can't release the reservation to the next level - that is maybe something that we will want to add. During query execution we can also implement different policies about how much reservation each query should hold onto. It's not clear to me yet what the right policy will be, particularly once we need to transfer reservation between different non-overlapping fragments. My feeling is that it makes sense to think about those policies more holistically in a later step when we can experiment with real queries and implement any ReservationTracker changes/extensions then (I don't see any obstacles to changing the policy with the current implementation). I added a TODO to DecreaseReservation() to decide on this and documented the current policy. I documented IncreaseReservation() and IncreaseReservationToFit(). Line 231: MemTracker* parent_mem_tracker_; > Or we could have ParentMemTracker() { return parent_ == NULL ? NULL: parent Done http://gerrit.cloudera.org:8080/#/c/3993/10/be/src/bufferpool/reservation-tracker.h File be/src/bufferpool/reservation-tracker.h: Line 114: /// before calling Close(). > is there a requirement that this tracker's children must have been closed b This is actually already DCHECKed by calling DecreaseReservationInternal() on the parent when closing. It should definitely be one since otherwise it's not clear what the expected behaviour of calling any method aside from Close() on the child is. Updated the comment. PS10, Line 130: '. > this one was missed: ... before calling this method. Done Line 178: > this would be easier to follow if we needed need both the unlocked and lock Removed IncreaseReservationInternal() by just having all callsites acquire the lock. PS10, Line 180: ns tr > false Done Line 194: void CheckConsistency() const; > In debug builds ... 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: 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
