Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13883 )
Change subject: IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and BufferedPRS impl ...................................................................... Patch Set 12: (2 comments) http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc File be/src/exec/buffered-plan-root-sink.cc: http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc@103 PS11, Line 103: DataSink::Close(state); : } : > Yeah, for the slow path I guess it doesn't matter much, but shouldn't there Yes, that's s the assumption now but that could change in the future. NotifyAll() here should work in either cases. It won't be fun debugging why users of this type of sink can sometimes hang. http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc@123 PS11, Line 123: // If the query was cancelled while the sink was waiting for rows to become available, : // or if the query was cancelled before the current call to GetNext, set eos and then : // return. The queue could be empty if the sink was closed > You do, I just decided to defer this part to a future patch. Added a TODO t May be I am misunderstanding the details but isn't the TODO part a core functionally ? Leaving this part out seems to mean this sink is broken / incomplete. -- To view, visit http://gerrit.cloudera.org:8080/13883 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b1bb4b9c6f6e92c70e8fbee6ccdf48c2f85b7be Gerrit-Change-Number: 13883 Gerrit-PatchSet: 12 Gerrit-Owner: Sahil Takiar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 26 Jul 2019 18:42:45 +0000 Gerrit-HasComments: Yes
