impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5169: Add support for async pins in buffer pool
Date Thu, 11 May 2017 00:31:00 GMT
Tim Armstrong has posted comments on this change.

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

Patch Set 10:

File be/src/runtime/

Line 411:   RETURN_IF_ERROR(read_page_->handle.GetBuffer(&read_buffer));
> don't we have to set the flag here too?
Good point. Moved the flag updating into a function in the Page object so it's easier to keep
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 BufferPo
I think it's relevant that it can block on I/O or fail, but some of the details arent.

PS10, Line 373: data_in_mem
> the "in mem" is really a detail hidden behind the BP abstraction.  How abou
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 rea
I inlined the logic into the callers (since it wouldn't be clear what this did aside from
call CancelRead() otherwise).

PS10, Line 236: UndoAsyncPin
> How about sticking with the list-oriented names, so something like: UndoMov

PS10, Line 241: async_write_in_flight
> wrong name

PS10, Line 247: write_
> same
File be/src/runtime/bufferpool/

Line 157:     Status status = ExtractBuffer(client, handle, &buffer);
> if the read had already finished before CancelAsyncPin() and had an error, 
Yes, there's no way the error status could propagate. I think this should be more obvious
with CancelAsyncPin() inlined.

PS10, Line 384: UndoAsyncPin
> Sticking with the list-oriented names, this is really UndoInFlightMoveToPin

PS10, Line 387: CancelAsyncPin
> page->CancelAsyncPin() isn't really canceling the pin, it's cancelling the 
Yeah more-or-less. Inlined CancelAsyncPin() to avoid having to name the thing.

PS10, Line 393: it doesn't have valid data.
> is that true? the read may have completed, right? I think this routine shou
Added a comment up the top explaining what the intended end-state is. Hopeful that's clearer.
File be/src/runtime/bufferpool/buffer-pool.h:

PS10, Line 413:  
> previously
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?
Reworded to be a bit less cryptic, hopefully.

I wanted to make it clear that it was valid to retry ReadAsync() even if the read failed.
In a lot of other places it's implicitly assumed that things shouldn't be retried once an
error occurs so I wanted to document that this was an exception.

We don't depend on this right now since we just cancel reads when we don't care about result,
but in an earlier iteration of the patch I was waiting on reads instead of cancelling them
and it was important because we might retry them later.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibdf074c1ac4405d6f08d623ba438a85f7d39fd79
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message