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 <[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
