Tim Armstrong has posted comments on this change. Change subject: IMPALA-3201: buffer pool header only ......................................................................
Patch Set 4: (21 comments) http://gerrit.cloudera.org:8080/#/c/3688/4/be/src/bufferpool/buffer-pool.h File be/src/bufferpool/buffer-pool.h: PS4, Line 62: one buffer pool client > pages can only have one client. Done PS4, Line 65: to hold another page. > should clarify that the buffer could also be used as a plain buffer. Done PS4, Line 72: buffers in memory. > buffers are always in memory. should say "have access to a certain number o Done PS4, Line 96: page : /// or buffer > nit: buffer or page Done PS4, Line 98: buffer > same Went with page thenbuffer since that was the order used in most of the text. PS4, Line 100: converting > what does this mean? extracting? I worded this slightly differently so that it stated less specifically that you could get a reference to the buffer to a page. I have a feeling that that particular API may evolve so don't want to document the full details up here. PS4, Line 100: page > buffer Done PS4, Line 104: Reference Counting > are the page handles actually referenced counted? or is this talking about That part of the design was removed as a result of other decisions. See below. PS4, Line 106: Multiple handles to a page can exist at any : /// given time, allowing for multiple handles to the same page to be used by the same : /// client. > is this allowed so that read and write iterators can reference the same pag Explained below but other design decisions made this complicated so I've just removed it. PS4, Line 125: CloseHandle > FreeBuffer() Done PS4, Line 126: Transfer > TransferBuffer() Done PS4, Line 127: operator > client Done Line 152: /// operations with the same Client, PageHandle or BufferHandle. > this seems more than an implementation detail. i.e clients can't know how Done PS4, Line 186: returns > delete Done PS4, Line 195: memory > buffer Done PS4, Line 197: page > page's buffer edited - page's data makes more sense to me - it's really the contents of the page that we're shuffling between memory and disk. PS4, Line 197: removed from memory > the buffer reclaimed. Done Line 208: void DuplicateHandle(const PageHandle& src, Client* client, PageHandle* dst); > why is this needed? is this to support simultaneous read and write iterato I think it would be a generally useful facility if you want to have memory shared between multiple places but without having to coordinate reservation bookkeeping using an additional external mechanism. E.g. suppose you're partitioning a row batch through a local exchange and you don't want to copy all of the underlying memory. With this you can just duplicate handles to all the underlying memory, attach it to the batches and send it to the destination operators. Referencing counting takes care of freeing the memory once all operators independently have closed their handles. I think that Without this you'd either have to deep-copy all the rows, or invent another mechanism off on the side to track that the memory is referenced by multiple operators. Other design decisions made this less useful (not transferring pages between operators) and harder to implement with sane invariants (the conversion to buffer handles) so I'll just delete it and we'll figure out a workaround if this comes up. PS4, Line 213: CloseHandle > Maybe we should call this ClosePage()? since we have BufferHandles as well. If we remove DuplicateHandle() this makes sense yes. Line 218: /// 'page_handle'. > what happens if the PageHandle has been duplicated? removed DuplicateHandle() PS4, Line 315: page's buffer > update Done -- To view, visit http://gerrit.cloudera.org:8080/3688 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id771dea2eb4c1aa13c30d59e8b184a7d1bca8d34 Gerrit-PatchSet: 4 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
