impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3202: implement spill-to-disk in new buffer pool
Date Fri, 13 Jan 2017 01:24:03 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message