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 04:22:23 GMT
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 <mj@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <todd@apache.org>
Gerrit-HasComments: Yes

Mime
View raw message