impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3201: headers and reservation logic for new buffer pool
Date Wed, 22 Jun 2016 22:32:09 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-3201: headers and reservation logic for new buffer pool
......................................................................


Patch Set 15:

(43 comments)

Focussed on the headers.  I don't see anything major needing to be changed in the interface.

http://gerrit.cloudera.org:8080/#/c/2569/15/be/src/bufferpool/buffer-pool.cc
File be/src/bufferpool/buffer-pool.cc:

PS15, Line 138: DCHECK
DCHECK_EQ


http://gerrit.cloudera.org:8080/#/c/2569/15/be/src/bufferpool/buffer-pool.h
File be/src/bufferpool/buffer-pool.h:

PS15, Line 39: and 
nit: end sentence and remove "and"


Line 47: ///
is the reservation tracker hierarchy also used to enforce max aggregate reservations across
all clients? i.e. enforce capacity_?


PS15, Line 70: it will be
             : /// allocated from the buffer pool
what does this mean?  before who allocates what?  what does "allocate" mean? shouldn't this
be talking about "pinning" instead?


PS15, Line 75: If a client pins the same page multiple time
have you found that a client will want to do that?
this is also true if multiple clients pin the same page simultaneously, right?


PS15, Line 99: buffer
page?


PS15, Line 101: buffer
page?


PS15, Line 101: buffers
page?


PS15, Line 124: capacity
any requirements on this being an exact multiple of min_page_len, or no?


Line 128:   /// arguments are invalid.
explicitly document the arguments.


PS15, Line 135: len
'len' here and else were (helps parse the comments IMO).


PS15, Line 137: If the preconditions are met
this kind of makes it sound like it's okay to call with the preconditions not met and a status
is returned. Let's just delete this.


Line 144:   /// page is accessible through either handle until the handle is closed.
document parameters explicitly.


PS15, Line 147: Close
what does this mean?  maybe define the "close" operation in the class comment.


PS15, Line 148: handles for a
maybe say "If this was the last handle for this page, then the page is cleaned up".  The current
wording sounds like you can close multiple handles via this interface.
but also, what does "cleaned up" mean? should this be talking about unpinning the page or
is this different?


PS15, Line 153: they have
it has


PS15, Line 154: If the preconditions are
              :   /// met
delete


PS15, Line 160: not
no longer


PS15, Line 160: is
becomes?


PS15, Line 162: not invalid
shoudl this say "not valid" or "invalid"?


Line 180:   /// Same as Unpin(), except the page's lock must be already held by the caller.
document parameters (here and else where)


Line 189:   /// Returns true and decrease 'remaining_capacity_' if successful.
i think this needs more explanation. is it increasing capacity_ or allocating more pages to
reach capacity, or something else?


Line 192:   BufferAllocator allocator_;
document what it's used for


Line 199:   const int64_t capacity_;
else where we refer to "capacity" as how much room the structure already has allocated (but
not necessarily used). e.g. row-batch.  here it means something a little different. in the
other way capacity is used, it seems to be more like the number of pages in pages_.  Maybe
we should have a different terminology for this?

i could be convinced otherwise though.


PS15, Line 204: remaining_capacity_
personally i find it easier to think about the positive than the negative. i.e. rather than
tracking how much is left, track how many bytes are currently allocated (by pages in pages_).
but okay to leave as is if this works out simplest.


Line 218: class BufferPool::Client {
who owns these objects? The client, like the PageHandle?


Line 239:   ReservationTracker* reservations_;
Are Clients and ReservationTrackers one-to-one? If so, why not put the ReservationTracker
in this class (rather than pointer)?


Line 257:   PageHandle& operator=(PageHandle&& src);
where are these two used? could we have a more explicit Transfer() routine instead?


Line 259:   bool is_open() const { return page_ != NULL; }
what does it mean for a PageHandle to be "open"? "closed"?


PS15, Line 264: .
... by this handle?


PS15, Line 277: Open
what does that  mean?


PS15, Line 278: buffer
page?


PS15, Line 283: pin count
which pin count? just the handle's count or the handle and the corresponding page's count?


PS15, Line 284: buf
why doesn't this just come from the page?


PS15, Line 286: .
for this handle or for the page as well?


PS15, Line 297: buf_
explain why this is duplicated here (from the page_).  To avoid the indirection i assume?


Line 300:   int64_t len_;
is this duplicated from the page_? explain that if so and why it's duplicated here.


Line 303:   int handle_pin_count_;
why do we have to track this? why isn't the page's pin count sufficient?


http://gerrit.cloudera.org:8080/#/c/2569/15/be/src/bufferpool/reservation-tracker.h
File be/src/bufferpool/reservation-tracker.h:

PS15, Line 42: maximum reservations can enforce
             : /// per-query limits.
i'm not sure how the first part of this sentences is relevant to this conclusion.  do you
also mean to talk about enforcing a per-process reservation limit, as well? i.e. would the
root maximum reservation correspond to the "capacity" of the buffer pool? or no?


PS15, Line 52: .
what's a case where a reservation tracker would not have a mem tracker?


PS15, Line 64: reservations
what reservations? only the ones for this root, right?


PS15, Line 87: at least
so is this allowed to increase reservation more than amount? but then how does a client know
how to undo this?


PS15, Line 107: RemainingReservation
Maybe UnusedReservation()?  "remaining" seems a little confusing because the reservation amount
still exists whether it's consumed or not.


-- 
To view, visit http://gerrit.cloudera.org:8080/2569
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
Gerrit-PatchSet: 15
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
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