Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201705110031.v4B0V0S8014827@ip-10-146-233-104.ec2.internal>
Date: Thu, 11 May 2017 00:31:00 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Dan Hecht <dhecht@cloudera.com>
Reply-To: tarmstrong@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5169=3A_Add_support_for_async_pins_in_buffer_pool=0A?=
In-Reply-To: <gerrit.1492013446000.Ibdf074c1ac4405d6f08d623ba438a85f7d39fd79@gerrit.cloudera.org>
References: <gerrit.1492013446000.Ibdf074c1ac4405d6f08d623ba438a85f7d39fd79@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Thu, 11 May 2017 00:31:05 -0000

Tim Armstrong has posted comments on this change.

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


Patch Set 10:

(14 comments)

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?
Good point. Moved the flag updating into a function in the Page object so it's easier to keep consistent.


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 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
Done


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'?
Done


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
Done


PS10, Line 241: async_write_in_flight
> wrong name
Done


PS10, Line 247: write_
> same
Done


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, 
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
Done


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.


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
Done


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?
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 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