Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/17195 )
Change subject: IMPALA-10584: Defer advancing read page when reservation is tight. ...................................................................... Patch Set 5: (1 comment) s http://gerrit.cloudera.org:8080/#/c/17195/5/be/src/runtime/buffered-tuple-stream.cc File be/src/runtime/buffered-tuple-stream.cc: http://gerrit.cloudera.org:8080/#/c/17195/5/be/src/runtime/buffered-tuple-stream.cc@728 PS5, Line 728: if (buffer_pool_client_->GetUnusedReservation() < last_attached_page_len_) { : // Since attached_to_output_batch is true and GetUnusedReservation() is less : // than last_attached_page_len_, the reader is most likely has not clean up the : // RowBatch where the read page is attached to. We defer advancing the read page : // until the next GetNext() call by the reader (see IMPALA-10584). : defer_advancing_read_page = true; : } > Your understanding is correct. Ideally, BufferedTupleStream should be 100% Ok, after some investigation, I'm not confident I can implement solution 2) safely. The refactoring will touch several code that I don't fully understand yet such as grouping-aggregator, partitioned-hash-join-builder, and analytic-eval-node. What if I change the logic into this instead: defer_advancing_read_page = num_pages_ <= 2; Basically, we avoid getting into situation where we ended up with only 1 read-write page while unpinning the stream. I think this is also a valid reason not to advance the read page (the other reason is that the reader has not freed the attached buffer). The "stealing" still won't happen and the DCHECK still won't hit. -- To view, visit http://gerrit.cloudera.org:8080/17195 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I16137b6e423f190f60c3115a06ccd0f77e9f585a Gerrit-Change-Number: 17195 Gerrit-PatchSet: 5 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Wed, 14 Apr 2021 21:27:30 +0000 Gerrit-HasComments: Yes
