impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3202: implement spill-to-disk in new buffer pool
Date Wed, 25 Jan 2017 21:19:40 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-3202: implement spill-to-disk in new buffer pool

Patch Set 13:

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"?
File be/src/runtime/bufferpool/

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.
File be/src/runtime/bufferpool/

PS13, Line 407: CHECK
did you mean DCHECK?
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?
File be/src/runtime/

Line 1222:   unique_lock<mutex> writer_lock(writer->lock_);
why is this removed?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c6119a4557da853fefa02e17c49d8be0e9fbe01
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message