Dan Hecht has posted comments on this change. Change subject: IMPALA-3202: implement spill-to-disk in new buffer pool ......................................................................
Patch Set 13: (10 comments) http://gerrit.cloudera.org:8080/#/c/5584/13/be/src/runtime/bufferpool/buffer-pool-internal.h File be/src/runtime/bufferpool/buffer-pool-internal.h: Line 67: /// the client before closing IMPALA-3202. yeah let's figure this out after this patch. i'm not sure such a tight coupling between reservation tracker and the buffer pool would be good, but maybe i'm misunderstanding your proposal. Anyway, let's discuss later. Line 241: return !buffer.is_open(); this is kind of a weird interface given that the answer may change immediately after the lock is dropped. Line 264: ConditionVariable write_complete_cv_; doesn't this pretty much duplicate the write_handle's condvar? would it be possible to just use write_handle->WaitForWrite() instead or does the locking get messy? (Also, in TmpFileMgr::WriteHandle::WriteComplete() should the notify_all() be moved after the callback executes (and maybe after the lock is dropped) to avoid waking up and immediately blocking on the lock?) PS13, Line 267: or evicted but how do you know if it's been evicted without calling IsEvicted() which takes the lock? I guess by checking the list memberships? PS13, Line 267: unpinned shouldn't this say "pinned" rather than "unpinned"? http://gerrit.cloudera.org:8080/#/c/5584/12/be/src/runtime/bufferpool/buffer-pool.cc File be/src/runtime/bufferpool/buffer-pool.cc: Line 464: } > I was thinking more about contention for clean_pages_lock_. But it should b yeah. i do think the clean_page_lock_ maybe a problem, but if so, we should fix it generally rather than just in this (relatively rare) case. http://gerrit.cloudera.org:8080/#/c/5584/13/be/src/runtime/bufferpool/buffer-pool.cc File be/src/runtime/bufferpool/buffer-pool.cc: PS13, Line 407: CHECK did you mean DCHECK? http://gerrit.cloudera.org:8080/#/c/5584/13/be/src/runtime/bufferpool/buffer-pool.h File be/src/runtime/bufferpool/buffer-pool.h: Line 224: /// any other operations for 'client'. why is it important to allow that? Line 232: /// concurrently with any other operations for 'src_client'. and here? http://gerrit.cloudera.org:8080/#/c/5584/12/be/src/runtime/disk-io-mgr.cc File be/src/runtime/disk-io-mgr.cc: Line 1222: unique_lock<mutex> writer_lock(writer->lock_); why is this removed? -- 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: 13 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
