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-5169: Add support for async pins in buffer pool
Date Wed, 10 May 2017 23:35:08 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-5169: Add support for async pins in buffer pool
......................................................................


Patch Set 10:

(14 comments)

This public abstraction looks good now. Mostly some renaming suggestions to clarify the internal
interfaces.

http://gerrit.cloudera.org:8080/#/c/6612/10/be/src/runtime/buffered-tuple-stream-v2.cc
File be/src/runtime/buffered-tuple-stream-v2.cc:

Line 411:   RETURN_IF_ERROR(read_page_->handle.GetBuffer(&read_buffer));
don't we have to set the flag here too?


http://gerrit.cloudera.org:8080/#/c/6612/10/be/src/runtime/buffered-tuple-stream-v2.h
File be/src/runtime/buffered-tuple-stream-v2.h:

PS10, Line 155: If the stream was previously unpinned, the page's data
              : ///     may not yet be in memory - the stream must call GetBuffer() (which
can block on
              : ///     I/O or fail with an I/O error) once the data is needed.
This seems like more of an implementation detail hidden behind the BufferPool abstraction.
Does the BTS or its clients really care about that?


PS10, Line 373: data_in_mem
the "in mem" is really a detail hidden behind the BP abstraction.  How about "retrieved_buffer",
or "referenced_buffer", or "buffer_referenced"?


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

PS10, Line 41: async_pin_in_flight
since all pins are async, how about just 'pin_in_flight'?


PS10, Line 107: CancelAsyncPin
CancelPinRead(), or CancelPinReadIO, or CancelReadIO? i.e. this doesn't really do anything
about pinning (that's handled by the impl_ level).


PS10, Line 236: UndoAsyncPin
How about sticking with the list-oriented names, so something like: UndoMoveEvictedToPinned()
or UndoInFlightMoveToPinned()?


PS10, Line 241: async_write_in_flight
wrong name


PS10, Line 247: write_
same


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

Line 157:     Status status = ExtractBuffer(client, handle, &buffer);
if the read had already finished before CancelAsyncPin() and had an error, does this still
always return ok?


PS10, Line 384: UndoAsyncPin
Sticking with the list-oriented names, this is really UndoInFlightMoveToPinned(), right?


PS10, Line 387: CancelAsyncPin
page->CancelAsyncPin() isn't really canceling the pin, it's cancelling the read I/O, right?

This routine is really the thing that "cancels" the in-progress pinning.


PS10, Line 393: it doesn't have valid data.
is that true? the read may have completed, right? I think this routine should be explained
more in terms of the write handle (scratch disk space) is still guaranteed to reflect the
page contents.  The buffer is in an unknown state (may or may not have the page's data) but
a reference to it definitely didn't escape).


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

PS10, Line 413:  
previously


http://gerrit.cloudera.org:8080/#/c/6612/10/be/src/runtime/tmp-file-mgr.h
File be/src/runtime/tmp-file-mgr.h:

PS10, Line 139: Even if the read fails, it is valid to call ReadAsync() again
              :     /// after this returns.
what does that mean?


-- 
To view, visit http://gerrit.cloudera.org:8080/6612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibdf074c1ac4405d6f08d623ba438a85f7d39fd79
Gerrit-PatchSet: 10
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