impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool
Date Wed, 07 Sep 2016 21:47:22 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message