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 Tue, 24 Jan 2017 06:08:02 GMT
Dan Hecht has posted comments on this change.

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


Patch Set 12:

(19 comments)

http://gerrit.cloudera.org:8080/#/c/5584/9/be/src/runtime/bufferpool/buffer-pool-internal.h
File be/src/runtime/bufferpool/buffer-pool-internal.h:

PS9, Line 255:  int64_t len;
> Made the change. I'm on the fence a bit about the name, since there is a di
But the callback does have to do something special for both cases.  And see my other questions
about whether these flags are needed.


http://gerrit.cloudera.org:8080/#/c/5584/10/be/src/runtime/bufferpool/buffer-pool-internal.h
File be/src/runtime/bufferpool/buffer-pool-internal.h:

PS10, Line 44: an unpinned page
i think saying: "a dirty unpinned page" would help make the transitions and the fact that
this is still a dirty page clearer.


PS10, Line 56: can
can be


http://gerrit.cloudera.org:8080/#/c/5584/9/be/src/runtime/bufferpool/buffer-pool.cc
File be/src/runtime/bufferpool/buffer-pool.cc:

Line 333:     buffer.Reset();
> Only if there's an accounting error. Obviously we should do everything poss
If you're not comfortable using DCHECK, let's at least add a quick comment explaining this
can only happen due to an accounting bug.  INTERNAL_ERROR code implies that, but given it's
not a common pattern, the comment seems warranted.

But why would a DCHECK here be any more risky than other DCHECKs in the buffer pool that are
checking global buffer pool invariants?


Line 469:     cl.lock();
> Done
You missed these questions.


http://gerrit.cloudera.org:8080/#/c/5584/10/be/src/runtime/bufferpool/buffer-pool.cc
File be/src/runtime/bufferpool/buffer-pool.cc:

PS10, Line 534: The async. writes may have hit an error.
Isn't it more that *initiating* the writes may have hit an error, as opposed to the write
itself (that error would be reported to the callback).


http://gerrit.cloudera.org:8080/#/c/5584/12/be/src/runtime/bufferpool/buffer-pool.cc
File be/src/runtime/bufferpool/buffer-pool.cc:

Line 191:   client->reservation_->AllocateFrom(page->len);
is the order of doing the pin_count / reservation bookkeeping and MoveToPinned() matter?


Line 217:   while (page_handle->pin_count() > 1) Unpin(client, page_handle);
this is okay, but why would a client want to extract a buffer that it had multiple pin counts
on? does that make sense to do from a client perspective?


Line 331:     bytes_evicted += buffer.len();
what's the difference between total_len and bytes_evicted?


PS12, Line 387: move it between lists.
shouldn't this really say "remove it from the in-flight list."?  though see comment in WriteCompleteCallback()
about whether we really need this flag.


Line 412:       file_group_->DestroyWriteHandle(move(page->write_handle));
why did we do this on line 395 as well? and why didn't that case need to hold the page lock?


Line 464:       page->repin_after_write = true;
i guess this is just an optimization? as in, alternatively, we could just wait for the write
to complete and then take the next case below (try RemoveCleanPage()).  Is it really worth
having this flag? i guess you're worried we'll commonly lose the race to RemoveCleanPage()
and the page will end up evicted if we didn't have this flag?  Given clean_pages_ is FIFO,
is this really worth optimizing for?


PS12, Line 470: atomically
atomically with respect to what? what would be wrong if we moved it to pinned_pages_ here
(and just made sure the callback didn't add it to clean page list)?


PS12, Line 500: Safe to touch the
              :   // page state without holding the lock variables below because no concurrent
operations
              :   // can modify evicted pages.
this comment applies to line 496 as well, right? So maybe move it up (or at least delete "below")?


PS12, Line 528: min_inflight_writes
min_inflight_bytes?  "writes" sounds like number of I/Os.


Line 560:       * file_group_->tmp_file_mgr()->NumActiveTmpDevices();
given that we don't know which tmp device a page is mapped to, how does this work? are we
just expecting it to average out over time?


Line 611:     in_flight_write_pages_.Remove(page);
DCHECK that only one of the after_write flags is set?


PS12, Line 614: !destroy_after_write
why do we need this guard?  why couldn't we just have DestroyPageInternal remove from the
clean list in this case after waiting for the write to complete?


Line 635: }
I don't feel too strongly, but why not just put the code from DebugStringLocked() inside DebugString()?


-- 
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: 12
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