impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
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:

(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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message