You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org> on 2018/01/10 21:52:16 UTC
[Impala-ASF-CR] IMPALA-6338: Fix flaky test profile fragment instances
Thomas Tauber-Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8997
Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances
......................................................................
IMPALA-6338: Fix flaky test_profile_fragment_instances
test_profile_fragment_instances checks that, once all the results have
been returned, every fragment instance appears in the query profile
for a query that internally cancels fragment instances that are still
executing when the results have been fully returned.
Every fis is guaranteed to send a profile to the coordinator in
Finalize(), but previously fragment profiles were not applied by the
coordinator if the backend was 'done', defined as either all instances
having completed or one has entered an error state (including
cancelled).
So, the test could fail by the following sequence:
- Some fragment for a particular backend sends an update to the
coordinator. 'returned_all_results_' is true, so the coordinator
responds indicating the the backend should cancel its remaining
fragments.
- Another fragment from that backend executes Finalize() and reports
that it was cancelled. This causes the coordinator to consider the
entire backend to be 'done'.
- A third fragment, which had not previously sent a report from the
reporting thread, from the same backend executes Finalize(). This
report will not be applied by the coordinator as the backend is
considered 'done', so this fragment will not appear in the final
profile.
The solution is to change the definition of 'done' to not include a
backend that has been cancelled but still has fragments that haven't
completed. This guarantees that for queries that complete successfully
and are cancelled internally, all fis will send a report and have it
applied by the coordinator before all results have been returned,
since if eos is true Coordinator::GetNext() calls
WaitForBackendCompletion(), which in this situation will now wait for
all fis to Finalize().
Returning results for queries that are cancelled by the user is
unaffected as the manual cancel path causes WaitForBackendCompletion().
Testing:
- Ran test_profile_fragment_instances in a loop with no failures.
I can reliably repro the original problem with a few carefully
placed sleeps.
Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M tests/query_test/test_observability.py
5 files changed, 25 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/8997/1
--
To view, visit http://gerrit.cloudera.org:8080/8997
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
Gerrit-Change-Number: 8997
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-6338: Fix flaky test profile fragment instances
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil, Joe McDonnell,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8997
to look at the new patch set (#3).
Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances
......................................................................
IMPALA-6338: Fix flaky test_profile_fragment_instances
test_profile_fragment_instances checks that, once all the results have
been returned, every fragment instance appears in the query profile
for a query that internally cancels fragment instances that are still
executing when the results have been fully returned.
Every fis is guaranteed to send a profile to the coordinator in
Finalize(), but previously fragment profiles were not applied by the
coordinator if the backend was 'done', defined as either all instances
having completed or one has entered an error state (including
cancelled).
So, the test could fail by the following sequence:
- Some fragment for a particular backend sends an update to the
coordinator. 'returned_all_results_' is true, so the coordinator
responds indicating the the backend should cancel its remaining
fragments.
- Another fragment from that backend executes Finalize() and reports
that it was cancelled. This causes the coordinator to consider the
entire backend to be 'done'.
- A third fragment, which had not previously sent a report from the
reporting thread, from the same backend executes Finalize(). This
report will not be applied by the coordinator as the backend is
considered 'done', so this fragment will not appear in the final
profile.
The solution is to change the definition of 'done' to not include a
backend that has been cancelled but still has fragments that haven't
completed. This guarantees that for queries that complete successfully
and are cancelled internally, all fis will send a report and have it
applied by the coordinator before all results have been returned,
since if eos is true Coordinator::GetNext() calls
WaitForBackendCompletion(), which in this situation will now wait for
all fis to Finalize().
Returning results for queries that are cancelled by the user is
unaffected as the manual cancel path causes WaitForBackendCompletion().
Testing:
- Ran test_profile_fragment_instances in a loop with no failures.
I can reliably repro the original problem with a few carefully
placed sleeps.
Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M tests/query_test/test_observability.py
5 files changed, 25 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/8997/3
--
To view, visit http://gerrit.cloudera.org:8080/8997
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
Gerrit-Change-Number: 8997
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6338: Fix flaky test profile fragment instances
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/8997 )
Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:
http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator-backend-state.h@206
PS2, Line 206: either because the client called Cancel() or because the coordinator
: /// initiated cancel because all the results were already returned
> It could also be because another backend hit an error.
Done
http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator-backend-state.h@243
PS2, Line 243: other than cancel
> We're making a base assumption that a CANCELLED status is only obtained as
I don't think that there are any situations in which a fragment instance would try to cancel the query without it being the result of some error, and of course if the coordinator sees an error it will cancel the query.
I did some grepping around to confirm this, but I'll get someone with more authority to chime in.
http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator.cc@926
PS2, Line 926: // If the query was cancelled by the user, don't process the update.
: if (query_status_.IsCancelled()) return Status::OK();
> Shouldn't we tell the backend to cancel itself if the query was cancelled b
Yes, we're relying on Cancel() having already sent cancels to all of the backends.
--
To view, visit http://gerrit.cloudera.org:8080/8997
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
Gerrit-Change-Number: 8997
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 23 Jan 2018 21:44:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6338: Fix flaky test profile fragment instances
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8997 )
Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8997
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
Gerrit-Change-Number: 8997
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Feb 2018 23:39:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6338: Fix flaky test profile fragment instances
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8997 )
Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:
http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator-backend-state.h@206
PS2, Line 206: either because the client called Cancel() or because the coordinator
: /// initiated cancel because all the results were already returned
It could also be because another backend hit an error.
Do we know of any other ways cancellation can be initiated? See more in my comment below.
http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator-backend-state.h@243
PS2, Line 243: other than cancel
We're making a base assumption that a CANCELLED status is only obtained as a result of the Coordinator setting and propagating it (for any reason).
But are we sure that's true (It would be ideal if that were true)? i.e. are we sure that there's no fragment instance cancelling itself (and hence the query) for some other reason?
Because if that was the case, then with this patch, a fragment instance trying to cancel the query wouldn't cause the query to be cancelled. Since the coordinator is assuming that if it sees a CANCELLED status from a fragment instance, that's due to the coordinator setting it.
http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator.cc@926
PS2, Line 926: // If the query was cancelled by the user, don't process the update.
: if (query_status_.IsCancelled()) return Status::OK();
Shouldn't we tell the backend to cancel itself if the query was cancelled by the user? Or are we relying on the Cancel() RPCs doing that?
--
To view, visit http://gerrit.cloudera.org:8080/8997
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
Gerrit-Change-Number: 8997
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Sat, 20 Jan 2018 00:45:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6338: Fix flaky test profile fragment instances
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8997 )
Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1864/
--
To view, visit http://gerrit.cloudera.org:8080/8997
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
Gerrit-Change-Number: 8997
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 19:45:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6338: Fix flaky test profile fragment instances
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8997 )
Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
The change LGTM.
The only other thing I want to bring up is that we're preferring a full profile over a quick teardown of a query. From a correctness point of view, that's sound. However, that may regress short queries in terms of total query time taken. But that's probably a topic for a much larger discussion that can be addressed as part of IMPALA-2990.
Since this change changes the lifecycle of a query a bit, it would be better if someone else had a look too.
http://gerrit.cloudera.org:8080/#/c/8997/3/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/8997/3/be/src/runtime/coordinator.cc@462
PS3, Line 462:
nit: Not your change, but we can get rid of this bad indent.
--
To view, visit http://gerrit.cloudera.org:8080/8997
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
Gerrit-Change-Number: 8997
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Jan 2018 23:41:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6338: Fix flaky test profile fragment instances
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8997 )
Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/8997
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
Gerrit-Change-Number: 8997
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 23:22:50 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6338: Fix flaky test profile fragment instances
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil, Joe McDonnell, Tim Armstrong, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8997
to look at the new patch set (#6).
Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances
......................................................................
IMPALA-6338: Fix flaky test_profile_fragment_instances
test_profile_fragment_instances checks that, once all the results have
been returned, every fragment instance appears in the query profile
for a query that internally cancels fragment instances that are still
executing when the results have been fully returned.
Every fis is guaranteed to send a profile to the coordinator in
Finalize(), but previously fragment profiles were not applied by the
coordinator if the backend was 'done', defined as either all instances
having completed or one has entered an error state (including
cancelled).
So, the test could fail by the following sequence:
- Some fragment for a particular backend sends an update to the
coordinator. 'returned_all_results_' is true, so the coordinator
responds indicating the the backend should cancel its remaining
fragments.
- Another fragment from that backend executes Finalize() and reports
that it was cancelled. This causes the coordinator to consider the
entire backend to be 'done'.
- A third fragment, which had not previously sent a report from the
reporting thread, from the same backend executes Finalize(). This
report will not be applied by the coordinator as the backend is
considered 'done', so this fragment will not appear in the final
profile.
The solution is to change the definition of 'done' to not include a
backend that has been cancelled but still has fragments that haven't
completed. This guarantees that for queries that complete successfully
and are cancelled internally, all fis will send a report and have it
applied by the coordinator before all results have been returned,
since if eos is true Coordinator::GetNext() calls
WaitForBackendCompletion(), which in this situation will now wait for
all fis to Finalize().
Returning results for queries that are cancelled by the user is
unaffected as the manual cancel path causes WaitForBackendCompletion().
Testing:
- Ran test_profile_fragment_instances in a loop with no failures.
I can reliably repro the original problem with a few carefully
placed sleeps.
Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
---
M be/src/common/status.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M tests/query_test/test_observability.py
6 files changed, 30 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/8997/6
--
To view, visit http://gerrit.cloudera.org:8080/8997
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
Gerrit-Change-Number: 8997
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6338: Fix flaky test profile fragment instances
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/8997 )
Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances
......................................................................
Patch Set 4:
(1 comment)
> (1 comment)
>
> The change LGTM.
>
> The only other thing I want to bring up is that we're preferring a
> full profile over a quick teardown of a query. From a correctness
> point of view, that's sound. However, that may regress short
> queries in terms of total query time taken. But that's probably a
> topic for a much larger discussion that can be addressed as part of
> IMPALA-2990.
>
> Since this change changes the lifecycle of a query a bit, it would
> be better if someone else had a look too.
Agree its important to thing about perf impact here, esp. for small queries.
I think ultimately we'll want to always end up constructing the complete runtime profile, since having full and accurate info in the profile is important for analysis, but that we may want to change the behavior where Fetch() blocks until the profile is complete.
http://gerrit.cloudera.org:8080/#/c/8997/3/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/8997/3/be/src/runtime/coordinator.cc@462
PS3, Line 462: St
> nit: Not your change, but we can get rid of this bad indent.
Done
--
To view, visit http://gerrit.cloudera.org:8080/8997
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
Gerrit-Change-Number: 8997
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 Jan 2018 18:37:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6338: Fix flaky test profile fragment instances
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8997 )
Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances
......................................................................
IMPALA-6338: Fix flaky test_profile_fragment_instances
test_profile_fragment_instances checks that, once all the results have
been returned, every fragment instance appears in the query profile
for a query that internally cancels fragment instances that are still
executing when the results have been fully returned.
Every fis is guaranteed to send a profile to the coordinator in
Finalize(), but previously fragment profiles were not applied by the
coordinator if the backend was 'done', defined as either all instances
having completed or one has entered an error state (including
cancelled).
So, the test could fail by the following sequence:
- Some fragment for a particular backend sends an update to the
coordinator. 'returned_all_results_' is true, so the coordinator
responds indicating the the backend should cancel its remaining
fragments.
- Another fragment from that backend executes Finalize() and reports
that it was cancelled. This causes the coordinator to consider the
entire backend to be 'done'.
- A third fragment, which had not previously sent a report from the
reporting thread, from the same backend executes Finalize(). This
report will not be applied by the coordinator as the backend is
considered 'done', so this fragment will not appear in the final
profile.
The solution is to change the definition of 'done' to not include a
backend that has been cancelled but still has fragments that haven't
completed. This guarantees that for queries that complete successfully
and are cancelled internally, all fis will send a report and have it
applied by the coordinator before all results have been returned,
since if eos is true Coordinator::GetNext() calls
WaitForBackendCompletion(), which in this situation will now wait for
all fis to Finalize().
Returning results for queries that are cancelled by the user is
unaffected as the manual cancel path causes WaitForBackendCompletion().
Testing:
- Ran test_profile_fragment_instances in a loop with no failures.
I can reliably repro the original problem with a few carefully
placed sleeps.
Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
Reviewed-on: http://gerrit.cloudera.org:8080/8997
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/common/status.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M tests/query_test/test_observability.py
6 files changed, 30 insertions(+), 18 deletions(-)
Approvals:
Tim Armstrong: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/8997
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
Gerrit-Change-Number: 8997
Gerrit-PatchSet: 7
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6338: Fix flaky test profile fragment instances
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil, Joe McDonnell,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8997
to look at the new patch set (#4).
Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances
......................................................................
IMPALA-6338: Fix flaky test_profile_fragment_instances
test_profile_fragment_instances checks that, once all the results have
been returned, every fragment instance appears in the query profile
for a query that internally cancels fragment instances that are still
executing when the results have been fully returned.
Every fis is guaranteed to send a profile to the coordinator in
Finalize(), but previously fragment profiles were not applied by the
coordinator if the backend was 'done', defined as either all instances
having completed or one has entered an error state (including
cancelled).
So, the test could fail by the following sequence:
- Some fragment for a particular backend sends an update to the
coordinator. 'returned_all_results_' is true, so the coordinator
responds indicating the the backend should cancel its remaining
fragments.
- Another fragment from that backend executes Finalize() and reports
that it was cancelled. This causes the coordinator to consider the
entire backend to be 'done'.
- A third fragment, which had not previously sent a report from the
reporting thread, from the same backend executes Finalize(). This
report will not be applied by the coordinator as the backend is
considered 'done', so this fragment will not appear in the final
profile.
The solution is to change the definition of 'done' to not include a
backend that has been cancelled but still has fragments that haven't
completed. This guarantees that for queries that complete successfully
and are cancelled internally, all fis will send a report and have it
applied by the coordinator before all results have been returned,
since if eos is true Coordinator::GetNext() calls
WaitForBackendCompletion(), which in this situation will now wait for
all fis to Finalize().
Returning results for queries that are cancelled by the user is
unaffected as the manual cancel path causes WaitForBackendCompletion().
Testing:
- Ran test_profile_fragment_instances in a loop with no failures.
I can reliably repro the original problem with a few carefully
placed sleeps.
Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M tests/query_test/test_observability.py
5 files changed, 27 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/8997/4
--
To view, visit http://gerrit.cloudera.org:8080/8997
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
Gerrit-Change-Number: 8997
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-6338: Fix flaky test profile fragment instances
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8997 )
Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8997/5/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:
http://gerrit.cloudera.org:8080/#/c/8997/5/be/src/runtime/coordinator-backend-state.h@223
PS5, Line 223: /// initiated cancel because all the results were already returned or an error was hit.
Are we assuming that status_ == CANCELLED implies that the cancellation was initiated by the coordinator? We should document that explicitly if so, and maybe also document Status::CANCELLED.
Would some of this logic be broken if one of the fragments failed without being cancelled and returned status CANCELLED? I believe that this shouldn't currently happen but it isn't currently documented.
Weirdly we use CANCELLED internally in some subsystems but if it leaks out to the fragment status it's a bug. We should probably switch to a different status for those use cases.
--
To view, visit http://gerrit.cloudera.org:8080/8997
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
Gerrit-Change-Number: 8997
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Feb 2018 20:00:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6338: Fix flaky test profile fragment instances
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/8997 )
Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8997/5/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:
http://gerrit.cloudera.org:8080/#/c/8997/5/be/src/runtime/coordinator-backend-state.h@223
PS5, Line 223: /// initiated cancel because all the results were already returned or an error was hit.
> Are we assuming that status_ == CANCELLED implies that the cancellation was
Yes, the assumption here is that status_ == CANCELLED here must have come from the coordinator - if a fragment reports that it's cancelled because it failed, these changes would mean that the coordinator would not initiate cancellation of the rest of the query as a result.
Added/cleaned up some comments around this.
--
To view, visit http://gerrit.cloudera.org:8080/8997
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
Gerrit-Change-Number: 8997
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Feb 2018 23:06:43 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6338: Fix flaky test profile fragment instances
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/8997 )
Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances
......................................................................
Patch Set 5: Code-Review+1
Carrying forward
--
To view, visit http://gerrit.cloudera.org:8080/8997
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed
Gerrit-Change-Number: 8997
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 Jan 2018 18:38:12 +0000
Gerrit-HasComments: No