impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool
Date Tue, 30 Aug 2016 01:36:34 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-3201: reservation implementation for new buffer pool

Patch Set 8:


Focused on the public interface.
File be/src/bufferpool/buffer-pool.h:

PS8, Line 266: s
given the way we use this term in reservation-tracker, seems this should be singular.
File be/src/bufferpool/reservation-tracker-counters.h:

Line 28:   /// This is set even if the reservation was not granted.
why is that?

PS8, Line 31: in bytes
here you do use units, while in ReservationTracker you try to stay resource agnostic (I prefer
this where units are specified -- see comments in reservation-tracker.h as well).

PS8, Line 35: Counter
these peak ones look like they're actually HighWaterMarkCounters.

Also, why have these peak counters, rather than just having the 'reservation' counter by a
high water mark counter that keeps track of this peak?
File be/src/bufferpool/reservation-tracker.h:

PS8, Line 40: and accounted against the tracker
            : /// directly
this is kind of vague.

PS8, Line 41: its
the child's 
to disambiguate.

Line 44: /// reservation (and perhaps increase reservations all the way to the root of the
given the various interpretations of "reservation" that we discussed, we need to spell out
the various terms more carefully.

Line 55: ///     child_reservations + used_reservation <= reservation.
maybe just introduce "unused_reservation" here so this can be equality.

Line 56: ///
let's have a quick explanation of thread-safety rules

PS8, Line 58: can

PS8, Line 58: integration
(and MemTracker)

PS8, Line 62:  included in memory limit checks
this could use some updating/clarification

PS8, Line 69: assume
require that

PS8, Line 71: assume

PS8, Line 79: .
I know you want to keep ReservationTracker for resources in general (as opposed to memory
specific), but we need some way to specify the units (readers won't know if it's bytes or
buffers, etc)

PS8, Line 83: counters_
the public interface shouldn't really refer to private stuff.  just explain what it adds to
the profile.

PS8, Line 85: MemTracker* mem_tracker
given our requirement that the top most link is the one that enforces limit, is this allowed
to be non-NULL?

PS8, Line 103: amount
also confusing to use without knowing units

PS8, Line 109: 'amount' is available to use
there is 'amount' of unused reservation.  (for consistency of terminology).

PS8, Line 123: back to the reservation
this can be confusing because it can be interpretted as incrementing reservation by 'account'
(if we had used the alternate terminology).  How about:

Free up 'amount' of previously allocated resources. The used reservation is decreased by 'amount'.

PS8, Line 127: memory
here you say memory (which I'm fine with, but let's be consistent one way or another).

Line 134:   /// Returns the amount of the reservation not used or given to childrens' reservations.
at this tracker

PS8, Line 134: or
also hard to parse whether the second condition is included or not in "unused reservation".
 how about:

... not used nor given to children's reservations.

Line 148:   /// Initializes 'counters_', storing the counters in 'profile'.
Wasn't sure what this meant without reading the code.  How about:

If 'profile' is not NULL, adds this tracker's set of runtime profile counters to 'profile'.
File be/src/util/runtime-profile-counters.h:

rather than adding these (and having to know to use them), why not always have valid runtime

this doesn't look needed

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message