Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4654: KuduScanner must return when ReachedLimit() ......................................................................
Patch Set 2: > Change looks good to me. > > As discussed, I think there is still one remaining bug with plans > like scan+topn where the coordinator may not necessarily cancel > fragments containing scans. In that case Close() is called on the > plan tree during tear down, but unlike other scan nodes the Kudu > scan node does not use done_ to terminate the scanner threads. For > non-Kudu scan nodes setting done_ triggers teardown of all scanner > threads. The scanner threads detect this indirectly via > ScannerContext::cancelled() which is checked on a per-row-batch > basis. I can't repro this yet (topn doesn't do it) so I'm hesitant to make the change in this CR, but I agree we should do it separately. It's worth digging into the topn case and seeing why it doesn't repro, but I can't spend too much time on that right now. I'll make a separate change to check done_ and that way we don't have to take it into a release branch until it has more bake time / time to find a repro. This does deserve more attention but given I can't find an obvious bug I can't work on it right now. > > Todd has a good point. Our limit check is broken. Agree to deal > with that everywhere in s separate change. Filed https://issues.cloudera.org/browse/IMPALA-4658 -- To view, visit http://gerrit.cloudera.org:8080/5493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaddd51111a1b2647995d68e6d37d0500b3a322de Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: No
