Dan Hecht has posted comments on this change. Change subject: IMPALA-3201: headers and reservation logic for new buffer pool ......................................................................
Patch Set 22: > I added an extra buffer interface that supports a subset of the > page functionality. The most straightforward way to implement this > is to make a wrapper around PageHandle that hides the other > functionality, so I went with that. If we want to go with this I > can do another pass to add some basic test coverage. > > I looked at changing the internal implementation to somehow have > different page and buffer concepts but it doesn't work out very > well. > > The two obvious ways to split out buffers internally are: > * Have separate internal buffer descriptor objects with their own > separate locks and state, with each pinned page pointing to a > buffer descriptor. Adding the extra locks and internal mappings > seems worth avoiding. What mappings are needed to necessity the lock? i.e. why can't a buffer descriptor just be a void * and length? > * Have completely separate internal state for pages and buffers, > along with a way to convert a page to a buffer internally. This > just leads to a lot of code duplication, plus trying to convert > things seems a little error prone. > I don't think of it as converting between the two. More of a layering (just like you had it) but where a buffer was a little bit more than a void * and could be retrieved from the buffer pool directly (rather than via a page). Maybe we'd need an operation like "unmap page" (or some other name) which takes a page and breaks the page -> buffer mapping, and returns the underlying buffer. I think we should think of the reservation as being associated with the buffers not pages. An operator (or subsystem like diskiomgr) can spend its reservation either to get a raw buffer or to back a page with a buffer. > E.g. what if we re-pinned a page that had a write in-flight, and we > want to convert it to a buffer, which has no notion of I/O. We'll > likely need to do some synchronisation on the page object when the > write completes, but if we converted it to a buffer then the page > has gone away. Actually that scenario is a problem for the first > approach too. > I think this would block in the "unmap page" call to wait for the write to complete (or just cancel the write at this point, or probably better, during the re-pin operation, if possible). Agree this may add some complexity though, and stems from wanting to do the write async (as opposed to synchronously during unpin which Mostafa has advocated for). > In general I think trying to convert or remap pages to buffers > internally complicates the lifecycle of internal descriptors. -- To view, visit http://gerrit.cloudera.org:8080/2569 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6 Gerrit-PatchSet: 22 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: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: No
