Marcel Kornacker 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 guarantees of the buffer pool, without comments about what clients might be doing or what might be going on under the covers. possibly as an introductory section to this class comment, if we feel we also need the expanded version. 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 engineering practices in class comments. Line 109: /// The invariants are as follows: update this section 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 particularly clear. 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 involved. Line 138: /// * If the operator is done with the page, it can call DestroyBuffer() to destroy the why DestroyBuffer? Line 203: /// buffers or disk storage backing the page is freed. Idempotent. should this be idempotent (but unpinning an unpinned page isn't)? 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 reservations? Line 277: int64_t len() const; is this legal for unpinned pages? -- 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
