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-4674: Part 1: port BufferedTupleStream to BufferPool
Date Wed, 15 Mar 2017 19:24:41 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4674: Part 1: port BufferedTupleStream to BufferPool
......................................................................


Patch Set 10:

(19 comments)

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

PS9, Line 322: 
!has_read_iterator() ?


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

Line 101:   for (const Page& page : pages_) {
will this be too expensive, even in debug builds (and we should make sure this loop is compiled
out of release builds)?


PS10, Line 125: read_page_ == pages_.end()
has_read_iterator() and switch clauses?


Line 129:   }
does it make sense to print the write page as well?


PS10, Line 149:   // This must be the first call to PrepareForWrite().
Maybe say, this must be the first iterator? i.e. you can't call this function after starting
a read iteration or read/write iteration, either, right?


PS10, Line 158:   // This must be the first call to PrepareForWrite().
same


PS10, Line 205: if
when
(sorry, i wrote the comment but 'if' sounds confusing since it's not currently pinned)


PS10, Line 211: If
When


PS10, Line 258: get_extra_reservation
it's unfortunate we need this. i wonder if moving the reservation logic (and the ResetWritePage()
call) to the callers would be simpler?

In any case, if we leave it here, how about calling this parameter 'get_read_reservation'?
also see next comment.


PS10, Line 272:  bool pin_for_read = has_read_iterator() && pinned_;
              :   get_extra_reservation |= pin_for_read;
this is still pretty confusing. if you don't move the reservation logic completely into the
callers, it may still make sense to move the calculation 'pinned_ && has_read_iterator()'
into AddRowSlow(). The other callsites are known to be either static true or false. 

THen you would also move the PinPage() call into the caller (using PinPageIfNeeded()), which
seems to make sense since in the case of PrepareForReadWrite(), you really want PrepareForReadInternal()
to do that anyway, no?  Or at least you could move this after the write iterator is set up
and still use PinPageIfNeeded().


Line 283:   if (pin_for_read) RETURN_IF_ERROR(PinPage(&new_page));
see comment above.


Line 298:     int64_t row_size, bool get_extra_reservation, bool* got_page) noexcept {
this wrapper now seems unnecessary and not helpful for readability because NewWritePage()
has no other callers.


PS10, Line 339: read_page_ == pages_.end()
!has_read_iterator()?


Line 379:   ResetReadPage();
this seems to make more sense to do in PrepareForRead() since that's the only case you can
possible have a read iterator, right? and that assumption is built into the reservation management
code.


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

PS10, Line 217: got_buffer
maybe this should be 'got_reservation' now, and explained in terms of reservation. or at least
got_page?


PS10, Line 228: got_buffer
same


PS10, Line 496: get_extra_reservation
this needs explanation, but first see comments in the cc file about it.


Line 503:       bool* got_page) noexcept WARN_UNUSED_RESULT;
seems like we can put this code in NewWritePage() now.


PS10, Line 509: got_buffer
got_page?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bb47a818564ac19a6be4ca88db0578f4ea0b709
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