Tim Armstrong has posted comments on this change. Change subject: IMPALA-3202: implement spill-to-disk in new buffer pool ......................................................................
Patch Set 7: (15 comments) 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 Yes they are substates of Unpinned. I reorganised to make it clearer that they are substates. Line 79: boost::lock_guard<boost::mutex> lock(lock_); > DCHECK_GT(page->pin_count, 0)? Done. Moved to buffer-pool.cc since it now needs the non-forward-declaration of Page. PS7, Line 94: UnpinPage > This name (and "RepinPage") is a bit confusing because it doesn't actually The new names are clearer, thanks. Line 104: Status EvictBeforeAllocation(ReservationTracker* reservation, int64_t allocation_len); I realised that Evict is a misnomer here. It's really ensuring that enough pages get moved to the clean state. 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)? Done Line 457: // Let WriteCompleteCallback() move it atomically to 'pinned_pages_'. > why would the callback move it to pinned list rather than clean list? oh, m Yeah I didn't love the flag solution, but doing it this way avoid duplicating some of the logic in WriteCompleteCallback(). Line 474: // the earlier IsEvicted() check. > why do we do the earlier check then? to avoid the pool lock? Exactly. I added a comment to the first check to explain that it's an optimisation. PS7, Line 485: client->impl_-> > isn't that 'this'? Yes, I missed fixing this while moving code from one class to another. 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 Done PS7, Line 170: All pages and buffers for the client : /// must be destroyed > All pages must be destroyed and buffers must be freed? Done Line 242: /// no locks lower in the lock acquisition order should be held by the caller. > where is the lock ordering documented? Yeah I thought moving it there would better separate the external/internal interfaces, even it's difficult to totally separate them in C++. PS7, Line 255: between lists is atomic > how is that, given that the clean page list is global (not client specific) I meant that there's no way another thread can see the intermediate state where the page was removed from a list in the client but not added to 'clean_pages_list_'. I'm not sure if there's a better way to express this. If the client lock is released before acquiring clean_pages_lock_ it makes some additional races possible (e.g. the cascaded if statements checking which list the page is in get more complicated). Line 400: > comment, including the fact that it's only valid to call when pinned. do w Added the comment. It seems useful as a convenience function to simplify client code (I can see cases where either mem_range() or data()/len() would lead to the cleanest client code). 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? I doubt it makes sense to do that. Removed this sentence. PS7, Line 213: intended to : /// be used for debugging. > this comment seems outdated now that you're using it for non-debugging. It's been used for non-debugging for a while I think :). Fixed it. -- 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-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
