Dan Hecht has posted comments on this change. Change subject: IMPALA-3202: implement spill-to-disk in new buffer pool ......................................................................
Patch Set 7: (14 comments) I need to dig deeper into the details, but had some comments about naming I wanted to put out there first. http://gerrit.cloudera.org:8080/#/c/5584/7/be/src/runtime/bufferpool/buffer-pool-internal.h File be/src/runtime/bufferpool/buffer-pool-internal.h: PS7, Line 38: its state I find this terminology a little confusing given that we also talk about a pages state as being "pinned" or "unpinned". The last four states below are really sub-states (w.r.t. the ClientImpl) for an unpinned page, right? maybe at least say "client state", and then below clarify the couple cases where it's not explicit that the state/list is for unpinned pages. Line 79: boost::lock_guard<boost::mutex> lock(lock_); DCHECK_GT(page->pin_count, 0)? PS7, Line 94: UnpinPage This name (and "RepinPage") is a bit confusing because it doesn't actually do the unpinnng, but is instead the client side part of the work needed to unpin. What do you think about naming these relative to the lists (a.k.a. "client states" of the page) they are operating on (similar to "AddNewPinnedPage" terminology)? For example: MoveToDirtyUnpinned() or similar. And likewise, MoveToPinned() instead of RepinPage()? http://gerrit.cloudera.org:8080/#/c/5584/7/be/src/runtime/bufferpool/buffer-pool.cc File be/src/runtime/bufferpool/buffer-pool.cc: Line 420: DCHECK(pinned_pages_.Contains(page)); DCHECK_EQ(page->pin_count, 0)? Line 457: // Let WriteCompleteCallback() move it atomically to 'pinned_pages_'. why would the callback move it to pinned list rather than clean list? oh, maybe the repin-after-write flag controls that? i guess the comment on line 449 kind of implies that but maybe be more explicit there. Line 474: // the earlier IsEvicted() check. why do we do the earlier check then? to avoid the pool lock? PS7, Line 485: client->impl_-> isn't that 'this'? http://gerrit.cloudera.org:8080/#/c/5584/7/be/src/runtime/bufferpool/buffer-pool.h File be/src/runtime/bufferpool/buffer-pool.h: Line 169: TmpFileMgr::FileGroup* file_group, RuntimeProfile* profile, Client* client); missing blank line PS7, Line 170: All pages and buffers for the client : /// must be destroyed All pages must be destroyed and buffers must be freed? Line 242: /// no locks lower in the lock acquisition order should be held by the caller. where is the lock ordering documented? oh, i saw it in the internal header. maybe add: "(see buffer-pool-internal.h)" PS7, Line 255: between lists is atomic how is that, given that the clean page list is global (not client specific)? Line 400: comment, including the fact that it's only valid to call when pinned. do we need both this and data()/len()? http://gerrit.cloudera.org:8080/#/c/5584/7/be/src/util/internal-queue.h File be/src/util/internal-queue.h: PS7, Line 39: InternalQueue InternalQueueBase? But is it even intended for this to be instantiated directly? PS7, Line 213: intended to : /// be used for debugging. this comment seems outdated now that you're using it for non-debugging. -- To view, visit http://gerrit.cloudera.org:8080/5584 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8c6119a4557da853fefa02e17c49d8be0e9fbe01 Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-HasComments: Yes
