Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4654: KuduScanner must return when ReachedLimit() ......................................................................
Patch Set 2: (3 comments) Alex, per our conversation about other cases- are you OK with investigating other potential related cases separately? I'd like to try to get this patch in since we know this case exists now and is very bad. http://gerrit.cloudera.org:8080/#/c/5493/2/be/src/exec/kudu-scanner.cc File be/src/exec/kudu-scanner.cc: Line 105: RETURN_IF_CANCELLED(state_); > Does this do anything? These should work when the query is actually sent the cancellation :) as we discussed in person, Sailesh found that the coordinator doesn't send the cancel rpc to fragments that have reported 'done' as they would in this case where the limit was reached. PS2, Line 200: scan_node_->ReachedLimit() > this isn't specific to Kudu, but it seems wrong that in this and other plac Yeah this is a good point, I'll file a separate JIRA for us to address how we're doing this everywhere. Doesn't seem to cause any issues here so I'd prefer we deal with it as a separate issue. http://gerrit.cloudera.org:8080/#/c/5493/2/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: Line 627: v.wait_for_metric("impala-server.num-fragments-in-flight", 0, timeout=30) > How sure are you this won't be flaky? Time seems tight to me. On my box this happened now nearly immediately (e.g. I can set this to 0), I also found this was successful w/ 30 on a test run. I don't see why it would take longer once this change is in? I'm happy to make it longer to be safe if you prefer though. -- 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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
