You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Henry Robinson (Code Review)" <ge...@cloudera.org> on 2017/02/13 23:19:32 UTC
[Impala-ASF-CR] IMPALA-4925: Cancel finstance if query has finished
Henry Robinson has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5987
Change subject: IMPALA-4925: Cancel finstance if query has finished
......................................................................
IMPALA-4925: Cancel finstance if query has finished
This patch is a partial fix for the issue where an finst would not
detect that it should cancel if the query limit had not been hit. It
changes the UpdateExecStatus() RPC to return a cancelled status to an
finst if the query has finished because it hit a limit.
For certain queries, this allows them to finish much more quickly than
they otherwise would. However, there's still a few-second delay for the
finst to pick up the cancellation signal, because there
UpdateExecStatus() RPC is only called every few seconds.
A complete fix would also call CancelInternal() when returned_all_results_
was set to true. That would be a much larger change. The improvement
here is to bound the delay between query completion and fragment
teardown to a few seconds.
Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
---
M be/src/runtime/coordinator.cc
1 file changed, 10 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/5987/1
--
To view, visit http://gerrit.cloudera.org:8080/5987
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-4925: Cancel finstance if query has finished
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4925: Cancel finstance if query has finished
......................................................................
Patch Set 2:
Thanks for the ping - I think this still worth doing, and is an even easier fix thanks to recent coordinator change, so here's an updated patch. I added a test.
--
To view, visit http://gerrit.cloudera.org:8080/5987
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4925: Cancel finstance if query has finished
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4925: Cancel finstance if query has finished
......................................................................
Patch Set 2:
Thanks for the review - any other comments?
--
To view, visit http://gerrit.cloudera.org:8080/5987
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4925: Cancel finstance if query has finished
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4925: Cancel finstance if query has finished
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5987/2/tests/query_test/test_lifecycle.py
File tests/query_test/test_lifecycle.py:
Line 94: assert end - now < 120, "Query took too long to complete: " + duration + "s"
> It seems like we could probably set this even lower, but this seems fine.
Agree it's a bit conservative, but I wanted to make sure ASAN builds didn't trigger any false positives.
--
To view, visit http://gerrit.cloudera.org:8080/5987
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4925: Cancel finstance if query has finished
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4925: Cancel finstance if query has finished
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5987
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4925: Cancel finstance if query has finished
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4925: Cancel finstance if query has finished
......................................................................
Patch Set 2: Code-Review+2
Included patch for IMPALA-5670 broke some tests. Rebased to remove it, carry +2.
--
To view, visit http://gerrit.cloudera.org:8080/5987
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4925: Cancel finstance if query has finished
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4925: Cancel finstance if query has finished
......................................................................
Patch Set 2: Code-Review+2
Oops I mean to +2
--
To view, visit http://gerrit.cloudera.org:8080/5987
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4925: Cancel finstance if query has finished
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4925: Cancel finstance if query has finished
......................................................................
Patch Set 1:
Is this still active?
--
To view, visit http://gerrit.cloudera.org:8080/5987
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4925: Cancel finstance if query has finished
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has submitted this change and it was merged.
Change subject: IMPALA-4925: Cancel finstance if query has finished
......................................................................
IMPALA-4925: Cancel finstance if query has finished
This patch is a partial fix for the issue where an finst would not
detect that it should cancel if the query limit had not been hit. It
changes the UpdateExecStatus() RPC to return a cancelled status to an
finst if the query has finished because it hit a limit.
For certain queries, this allows them to finish much more quickly than
they otherwise would. However, there's still a few-second delay for the
finst to pick up the cancellation signal, because there
UpdateExecStatus() RPC is only called every few seconds.
A complete fix would also call CancelInternal() when returned_all_results_
was set to true. That would be a much larger change. The improvement
here is to bound the delay between query completion and fragment
teardown to a few seconds.
Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Reviewed-on: http://gerrit.cloudera.org:8080/5987
Tested-by: Impala Public Jenkins
Reviewed-by: Henry Robinson <he...@cloudera.com>
---
M be/src/runtime/coordinator.cc
M tests/query_test/test_lifecycle.py
2 files changed, 30 insertions(+), 1 deletion(-)
Approvals:
Impala Public Jenkins: Verified
Henry Robinson: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/5987
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4925: Cancel finstance if query has finished
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4925: Cancel finstance if query has finished
......................................................................
Patch Set 2:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/874/
--
To view, visit http://gerrit.cloudera.org:8080/5987
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4925: Cancel finstance if query has finished
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has uploaded a new patch set (#2).
Change subject: IMPALA-4925: Cancel finstance if query has finished
......................................................................
IMPALA-4925: Cancel finstance if query has finished
This patch is a partial fix for the issue where an finst would not
detect that it should cancel if the query limit had not been hit. It
changes the UpdateExecStatus() RPC to return a cancelled status to an
finst if the query has finished because it hit a limit.
For certain queries, this allows them to finish much more quickly than
they otherwise would. However, there's still a few-second delay for the
finst to pick up the cancellation signal, because there
UpdateExecStatus() RPC is only called every few seconds.
A complete fix would also call CancelInternal() when returned_all_results_
was set to true. That would be a much larger change. The improvement
here is to bound the delay between query completion and fragment
teardown to a few seconds.
Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
---
M be/src/runtime/coordinator.cc
M tests/query_test/test_lifecycle.py
2 files changed, 30 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/5987/2
--
To view, visit http://gerrit.cloudera.org:8080/5987
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4925: Cancel finstance if query has finished
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4925: Cancel finstance if query has finished
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5987/2/tests/query_test/test_lifecycle.py
File tests/query_test/test_lifecycle.py:
Line 94: assert end - now < 120, "Query took too long to complete: " + duration + "s"
It seems like we could probably set this even lower, but this seems fine.
--
To view, visit http://gerrit.cloudera.org:8080/5987
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4925: Cancel finstance if query has finished
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4925: Cancel finstance if query has finished
......................................................................
Patch Set 3:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/877/
--
To view, visit http://gerrit.cloudera.org:8080/5987
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4925: Cancel finstance if query has finished
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4925: Cancel finstance if query has finished
......................................................................
Patch Set 1:
(2 comments)
The code change makes sense now that I've wrapped my head around it.
Can we add a test that exercises this code path? E.g. there was a query in the JIRA.
http://gerrit.cloudera.org:8080/#/c/5987/1//COMMIT_MSG
Commit Message:
Line 19: A complete fix would also call CancelInternal() when returned_all_results_
Is there a JIRA to track this.
http://gerrit.cloudera.org:8080/#/c/5987/1/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
Line 1514: if (!returned_all_results_ && !status.ok()) {
I feel like it would be clearer if !status.ok() came first so it was visually more obvious that this is an error-handling path.
Somewhat relatedly, the comment might be clearer if it was phrased as something like "Start cancellation if there was an error - unless all results are returned, in which case we can safely ignore the instance's status".
--
To view, visit http://gerrit.cloudera.org:8080/5987
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4925: Cancel finstance if query has finished
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4925: Cancel finstance if query has finished
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5987/1/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
Line 1547: if (!params.done && returned_all_results_) {
One line?
--
To view, visit http://gerrit.cloudera.org:8080/5987
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4925: Cancel finstance if query has finished
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4925: Cancel finstance if query has finished
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/5987
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4925: Cancel finstance if query has finished
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4925: Cancel finstance if query has finished
......................................................................
Patch Set 2: Verified-1
Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/874/
--
To view, visit http://gerrit.cloudera.org:8080/5987
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No