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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
