Dan Hecht 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. PS4, Line 65: to hold another page. should clarify that the buffer could also be used as a plain buffer. PS4, Line 72: buffers in memory. buffers are always in memory. should say "have access to a certain number of buffers" or something like that. PS4, Line 96: page : /// or buffer nit: buffer or page (for consistency in order) PS4, Line 98: buffer same PS4, Line 100: converting what does this mean? extracting? also you can get the BufferHandle that backs a pinned page without extracting. PS4, Line 100: page buffer PS4, Line 104: Reference Counting are the page handles actually referenced counted? or is this talking about pin counts? 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 page? or something else? PS4, Line 125: CloseHandle FreeBuffer() PS4, Line 126: Transfer TransferBuffer() PS4, Line 127: operator client Line 152: /// operations with the same Client, PageHandle or BufferHandle. this seems more than an implementation detail. i.e clients can't know how to use this interface without knowing the synchronization rules laid out here. So I'd remove "IMPLEMENTATION DETAILS" and rename the section "Synchronization" or "Thread-safety". PS4, Line 186: returns delete PS4, Line 195: memory buffer PS4, Line 197: removed from memory the buffer reclaimed. PS4, Line 197: page page's buffer Line 208: void DuplicateHandle(const PageHandle& src, Client* client, PageHandle* dst); why is this needed? is this to support simultaneous read and write iterators referencing the same page? Do we need both this and the ability to pin a particular PageHandle multiple times? PS4, Line 213: CloseHandle Maybe we should call this ClosePage()? since we have BufferHandles as well. Line 218: /// 'page_handle'. what happens if the PageHandle has been duplicated? PS4, Line 315: page's buffer update -- 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
