Tim Armstrong has posted comments on this change. Change subject: IMPALA-3201: buffer pool header only ......................................................................
Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/3688/5/be/src/bufferpool/buffer-pool.h File be/src/bufferpool/buffer-pool.h: Line 85: /// it is able to fulfill the reservations when needed, e.g. by flushing unpinned pages > i think it would be good to create a condensed version of the specific guar I deleted the middle paragraph here and cut down the other paragraphs, just to focus on the idea that reservations are necessary and sufficient for allocating memory. I think the method comments should be the canonical reference for more specific guarantees. Line 90: /// The buffer pool does not directly expose its internal data structures to clients, > opportunity for more brevity: no need to explain general software engineeri Done Line 109: /// The invariants are as follows: > update this section Done Line 113: /// * Pin() can be called on a pinned handle, incrementing the handle's pin count. > it sounds like you're mixing up 'pinned' and 'open'. the distinction isn't Done Line 124: /// destroy the handle and free the page and its buffer, or use TransferBuffer() to > from the perspective of the caller, it's not clear why/how a page is involv editing error, removed. Line 138: /// * If the operator is done with the page, it can call DestroyBuffer() to destroy the > why DestroyBuffer? Editing error - DestroyPage(). Fixed. Line 203: /// buffers or disk storage backing the page is freed. Idempotent. > should this be idempotent (but unpinning an unpinned page isn't)? The pins are referenced counted, so unpinning twice is different from unpinning once. We could change it so that unpinning an unpinned page has no effect, but I think that would make it easier to be sloppy. Idempotency of DestroyPage() is mainly to simplify cleanup logic so it doesn't have to check whether everything is open. Line 223: Status TransferBuffer(Client* src_client, BufferHandle* src, Client* dst_client, > can't the handle stay the same? what is the effect on the src's and dst's r Made the reservation implications of the ownership change explicit in the comment. No reason it can't be transferred in-place. I did it this way since usually it makes sense to move to a new handle when transferring ownership. E.g. attaching it to a row batch. This would discourage patterns where the ownership of a a particular member variable or vector of handles was ambiguous. Line 277: int64_t len() const; > is this legal for unpinned pages? Yeah, the length of the page's data still makes sense if the page is unpinned. -- 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: 5 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
