Tim Armstrong 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. * 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. 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. 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
