Tim Armstrong has posted comments on this change. Change subject: IMPALA-3201: buffer pool header only ......................................................................
Patch Set 6: (24 comments) http://gerrit.cloudera.org:8080/#/c/3688/6/be/src/bufferpool/buffer-pool.h File be/src/bufferpool/buffer-pool.h: PS6, Line 62: and : /// the corresponding buffer > how about deleting this now that BufferHandle is a first class construct? I think this still makes sense since you access the buffer via the page. PS6, Line 91: A new > nit: An Done PS6, Line 94: unclosed > what's the difference between unclosed and open? Same thing. Changed. PS6, Line 94: thatr > typo Done PS6, Line 100: DestroyHandle > DestroyPage() Done PS6, Line 110: CreateBuffer > AllocateBuffer() Done PS6, Line 118: () > nit: add period Done PS6, Line 120: free up memory reservations > would be nice to use terminology consistent with what we ended up with in R Done PS6, Line 123: > unused? available? Done PS6, Line 166: see BufferPool class comment > I just read the comment and don't remember where this was documented :) is Under "Pages, Buffers and Pinning". I moved it to its own section to be called outbetter. PS6, Line 167: > unused? available? Done PS6, Line 169: . > due to an I/O error? or something, to emphasize that these are only system Defined 'system errors' in "Reservations" section. PS6, Line 176: > available? unused? Done PS6, Line 177: error > maybe we should come up with a term for these kinds of errors to emphasize See above comment - "system errors" PS6, Line 181: Releases client's reservation > Decreases the client's usage of its reservation. or similar, since it doesn Done Line 189: void Unpin(Client* client, PageHandle* handle); > Pin/Unpin are both illegal on closed handles, correct? it's implied in Unp Done PS6, Line 191: '. > maybe say ... "if the handle is opened." to clarify how idempotency works ( Done PS6, Line 191: Then the page is destroyed > seems redundant with first sentence. also would be nice to explicitly say t Done PS6, Line 191: Destroy's > Destroy Done Line 192: /// buffers or disk storage backing the page is freed. Idempotent. > what happens if you destroy a pinned page? that decreases reservation usag Done PS6, Line 207: free up the reservation > decrease the reservation used by, or similar. Done PS6, Line 211: frees > decreases reservation usage Done Line 212: /// reservation in 'src_client'. > what happens to 'src' handle, is it closed? what is 'src' is a closed handl Right. Added. PS6, Line 271: or NULL if the page is unpinned > shouldn't this be illegal for unpinned pages? (just like data() specificat 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: 6 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
