impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4654: KuduScanner must return when ReachedLimit()
Date Wed, 14 Dec 2016 17:56:22 GMT
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 <mj@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <todd@apache.org>
Gerrit-HasComments: No

Mime
View raw message