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) 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; : } > Maybe I'm misunderstanding something. This still seems tricky for me. I agr Your understanding is correct. Ideally, BufferedTupleStream should be 100% aware whether its client has freed the attached buffer or not. It raise a question though on how to do that, because the stream lost reference to the buffer once it attach the buffer to output row batch. In relation of BufferedPlanRootSink and SpillableRowBatchQueue, I can think of 2 solution: 1). Track the amount of both used and unused reservation at the end of GetNext() call. We can then check the reservation amount again in UnpinStream() to determine whether client has freed the buffer or not. However, this can be tricky and lead to more corner cases such as what if we increase/reduce the total reservation in between? 2). Add an explicit method in BufferedTupleStream to signal the free. SpillableRowBatchQueue then need similar method as well to chain the signal down from BufferedPlanRootSink to BufferedTupleStream. The mechanics might becomes awkward, but it maintain correctness. I'll try to implement solution 2) first. -- 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 18:13:35 +0000 Gerrit-HasComments: Yes
