Dan Hecht has posted comments on this change. Change subject: IMPALA-3201: buffer pool header only ......................................................................
Patch Set 6: (25 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? PS6, Line 91: A new nit: An (to be consistent with PageHandle sentence). PS6, Line 94: unclosed what's the difference between unclosed and open? PS6, Line 94: thatr typo PS6, Line 100: DestroyHandle DestroyPage() PS6, Line 104: releases reservations One way to interpret this is that the reservation is adjusted, but it's not. Let's use the terminology we have in ReservationTracker: ... always increases/decreases reservation usage. or something similar. PS6, Line 110: CreateBuffer AllocateBuffer() PS6, Line 118: () nit: add period PS6, Line 120: free up memory reservations would be nice to use terminology consistent with what we ended up with in ReservationTracker: ... decrease its reservation usage ... i.e. it's not giving away (or freeing) the reservation itself; it still has the same amount of reservation afterwords, but less usage. PS6, Line 123: unused? available? PS6, Line 166: see BufferPool class comment I just read the comment and don't remember where this was documented :) is it still there? PS6, Line 167: unused? available? PS6, Line 169: . due to an I/O error? or something, to emphasize that these are only system errors that can prevent it from succeeding. PS6, Line 176: available? unused? PS6, Line 177: error maybe we should come up with a term for these kinds of errors to emphasize what this is really trying to say? PS6, Line 181: Releases client's reservation Decreases the client's usage of its reservation. or similar, since it doesn't actually release the reservation itself, just the current usage of it. Line 189: void Unpin(Client* client, PageHandle* handle); Pin/Unpin are both illegal on closed handles, correct? it's implied in Unpin since closed would have a pincount of 0, but maybe call that out as a precondition, especially since DestroyPage() allows closed handles on input. PS6, Line 191: Then the page is destroyed seems redundant with first sentence. also would be nice to explicitly say this closes the handle. PS6, Line 191: Destroy's Destroy PS6, Line 191: '. maybe say ... "if the handle is opened." to clarify how idempotency works (open -> closed, closed -> closed). Line 192: /// buffers or disk storage backing the page is freed. Idempotent. what happens if you destroy a pinned page? that decreases reservation usage, right? would be worth calling that out explicitly. PS6, Line 207: free up the reservation decrease the reservation used by, or similar. also would be nice to say this closes 'handle'. also is this idempotent like DestroyPage()? This needs to be specified (i.e. does 'handle' have to be an open handle?) PS6, Line 211: frees decreases reservation usage Line 212: /// reservation in 'src_client'. what happens to 'src' handle, is it closed? what is 'src' is a closed handle on input? and I assume 'dst' must be closed on input, right? PS6, Line 271: or NULL if the page is unpinned shouldn't this be illegal for unpinned pages? (just like data() specification.) -- 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
