Dan Hecht has posted comments on this change. Change subject: IMPALA-3201: reservation implementation for new buffer pool ......................................................................
Patch Set 10: (12 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: MemLimitExceeded Shouldn't we introduce a Reservation-specific error code? Okay to defer for now but wondering what the plan is. Line 101: status.AddDetail("Could not get initial reservation"); should this include 'initial_reservation' and maybe some label to make it easier to debug? Line 143: // the reservation. This comment is confusing because it sounds like this is a precondition to calling Close(), but then line 144 does what it says has to be done before closing. Reword. Line 145: mem_tracker_ = NULL; the method comment claims that this unlinks from parent, but parent_ is not reset. Should it? 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); > IncreaseReservation() seems like the better primitive since you can impleme My motivation for bringing it up is because it'd be nice to remove the !use_existing_reservation case, which would simplify things. So making this private doesn't help. Also, related, it seems a bit weird that IncreaseReservation() will use a parent's pre-existing reservation but then DecreaseReservation() will force the parent to relinquish the reservation. Why is that the right thing? And given that Decrease always forces the parent to give back reservation, when would Increase even have the opportunity to use the parent's (already acquired) reservation? I guess the expectation is that some clients will get a reservation at, say, the exec-node level, and then later use that reservation at a child to the exec-node? But then, again, how does that fit in with the Decrease behavior? In any case, this difference is subtle and needs to be called out in the Increase and Decrease methods (I.e. they don't explain how the hierarchy comes into play). Alternatively, I wonder if instead of DecreaseReservation() we should just have a ClearReservation(), which seems to have clearer semantics w.r.t. the parent reservation. But I'm first looking to understand how DecreaseReservation() will be used. Line 231: > Removed it. The checks get a little more complicated (have to check if both Or we could have ParentMemTracker() { return parent_ == NULL ? NULL: parent_->mem_tracker_; } but I'm fine with the new code too. 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 before calling Close() on it? PS10, Line 130: '. this one was missed: ... before calling this method. Line 178: int64_t bytes, bool use_existing_reservation, bool is_child_reservation); this would be easier to follow if we needed need both the unlocked and locked variants, but it's not immediately obvious how to get rid of the locked given the call paths from Init (and Close for Decrease). But maybe you can think of a clean way. PS10, Line 180: error false Line 194: /// Check the internal consistency of the ReservationTracker. In debug builds ... or mention that this results in DCHECKs. Might be even clearer to rename it DCheckConsistency(). Or DCheckInvariants(). http://gerrit.cloudera.org:8080/#/c/3993/10/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: PS10, Line 208: thanks for fixing this, it always confused me. -- 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: 10 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
