impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <>
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.


To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaddd51111a1b2647995d68e6d37d0500b3a322de
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: No

View raw message