You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org> on 2020/07/19 16:43:42 UTC
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Wenzhe Zhou has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16215
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
IMPALA-5746: Cancel all queries scheduled by failed coordinators
Executor registers the updating of cluster membership. When coordinators
are absence from the active cluster membership list, executer cancels
all the running fragments of the queries which are scheduled by the
inactive coordinator since the executer cannot send results back to
the inactive/failed coordinators. This makes executers quickly release
the resources allocated for those running fragments.
Testing:
- Added new test case TestRestart::test_kill_coordinator.
- Passed the core test.
Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_restart_services.py
10 files changed, 129 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/16215/2
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6643/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Sun, 19 Jul 2020 17:11:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
IMPALA-5746: Cancel all queries scheduled by failed coordinators
Executor registers the updating of cluster membership. When coordinators
are absence from the active cluster membership list, executer cancels
all the running fragments of the queries which are scheduled by the
inactive coordinator since the executer cannot send results back to
the inactive/failed coordinators. This makes executers quickly release
the resources allocated for those running fragments.
Testing:
- Added new test case TestRestart::test_kill_coordinator.
- Passed the core test.
Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_restart_services.py
10 files changed, 127 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/16215/3
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/exec-env.cc@554
PS3, Line 554: server->CancelQueriesOnFailedBackends(current_backend_set);
> Yeah for this and your other comment below, probably the cleanest thing is
I was thinking to reuse the backend set and save a loop with one callback function to handle two tasks. But it seems not clean. Will change to register two separate callback functions as suggested by Thomas.
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/exec-env.cc@560
PS3, Line 560: cluster_membership_mgr_->RegisterUpdateCallbackFn(
: [server](ClusterMembershipMgr::SnapshotPtr snapshot) {
: std::unordered_set<BackendIdPB> current_backend_set;
: for (const auto& it : snapshot->current_backends) {
: current_backend_set.insert(it.second.backend_id());
: }
: server->CancelQueriesOnFailedBackends(current_backend_set);
: });
> not necessarily related to your change, but it would be nice to make this c
Will fix it.
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/query-exec-mgr.cc
File be/src/runtime/query-exec-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/query-exec-mgr.cc@222
PS3, Line 222: // TODO: create cancellation task queue and working thread to run cancellation tasks
: // on a separate thread. If the queue is full, ignore the cancellations since we'll
: // be able to process them on the next heartbeat instead.
:
: for (auto& qs : to_cancel) {
: VLOG(1) << "CancelQueriesForFailedCoordinators(): cancel query " << qs->query_id();
: qs->Cancel();
: qs->is_coord_active_.Store(false);
: ReleaseQueryState(qs);
: }
> I had suggested not to put this in ImpalaServer because its primarily the c
Will define a new thread pool owned by QueryExecMgr.
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/query-exec-mgr.cc@222
PS3, Line 222: // TODO: create cancellation task queue and working thread to run cancellation tasks
: // on a separate thread. If the queue is full, ignore the cancellations since we'll
: // be able to process them on the next heartbeat instead.
:
: for (auto& qs : to_cancel) {
: VLOG(1) << "CancelQueriesForFailedCoordinators(): cancel query " << qs->query_id();
: qs->Cancel();
: qs->is_coord_active_.Store(false);
: ReleaseQueryState(qs);
: }
> if you move this into impala-server.cc you can use the ImpalaServer::Cancel
The item type and work_function which runs for each item are set when creating a thread pool. It seems that one thread pool cannot serve two different types of tasks. The class CancellationWork is defined to handle cancellation of remote running fragments for coordinator. Maybe we should define a separate cancellation work for executer to cancel local running fragments and create a separate thread pool with one thread.
http://gerrit.cloudera.org:8080/#/c/16215/3/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:
http://gerrit.cloudera.org:8080/#/c/16215/3/common/thrift/ImpalaInternalService.thrift@607
PS3, Line 607: 27: optional Types.TUniqueId coord_backend_id
Will change coord_backend_id data type as UniqueIdPB and move it to FragmentInstanceExecStatusPB in control_service.proto.
http://gerrit.cloudera.org:8080/#/c/16215/3/tests/custom_cluster/test_restart_services.py
File tests/custom_cluster/test_restart_services.py:
http://gerrit.cloudera.org:8080/#/c/16215/3/tests/custom_cluster/test_restart_services.py@241
PS3, Line 241: def test_kill_coordinator(self):
> This is an okay place for this test (obviously we have a lot of different t
Yes, it's better to move it to test_process_failures.py. Will do.
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Jul 2020 20:30:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6690/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 4
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Jul 2020 19:15:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
IMPALA-5746: Cancel all queries scheduled by failed coordinators
Executor registers the updating of cluster membership. When coordinators
are absence from the active cluster membership list, executer cancels
all the running fragments of the queries which are scheduled by the
inactive coordinator since the executer cannot send results back to
the inactive/failed coordinators. This makes executers quickly release
the resources allocated for those running fragments to be canceled.
Testing:
- Added new test case TestProcessFailures::test_kill_coordinator
and ran the test case as following command:
./bin/impala-py.test tests/custom_cluster/test_process_failures.py\
::TestProcessFailures::test_kill_coordinator \
--exploration_strategy=exhaustive.
- Passed the core test.
Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/test-env.cc
M common/protobuf/control_service.proto
M tests/custom_cluster/test_process_failures.py
9 files changed, 183 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/16215/4
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 4
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16215/2/tests/custom_cluster/test_restart_services.py
File tests/custom_cluster/test_restart_services.py:
http://gerrit.cloudera.org:8080/#/c/16215/2/tests/custom_cluster/test_restart_services.py@239
PS2, Line 239: @
> flake8: E303 too many blank lines (2)
Fixed
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Sun, 19 Jul 2020 16:50:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 6
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 Jul 2020 16:35:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16215/4/be/src/runtime/query-exec-mgr.h
File be/src/runtime/query-exec-mgr.h:
http://gerrit.cloudera.org:8080/#/c/16215/4/be/src/runtime/query-exec-mgr.h@38
PS4, Line 38: /// query execution.
Could you update this comment to include:
- A little about the new cancellation stuff
- A little about the locking protocol here (i.e. that you have to hold the shard lock in order to increment the refcnt for a QueryState safely)
http://gerrit.cloudera.org:8080/#/c/16215/4/be/src/runtime/query-exec-mgr.h@97
PS4, Line 97: boost::scoped_ptr
We prefer to use std::unique_ptr in new code
http://gerrit.cloudera.org:8080/#/c/16215/4/be/src/runtime/query-exec-mgr.cc
File be/src/runtime/query-exec-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/16215/4/be/src/runtime/query-exec-mgr.cc@61
PS4, Line 61: retried on the next statestore heartbeat
I don't think this is true (I see that you copied this comment from impala-server.cc and I think its also incorrect there, though it may have been correct in 2013 when it was written), though you might check me by changing the queue size to 1 and seeing if your test passes.
I'm pretty sure the callbacks will only get called on statestore updates that actually contain changes to the membership, so if a node is removed we'll only run CancelQueriesForFailedCoordinators once.
That should be okay though - with such a large queue size its unlikely we'll hit the limit (and if you do, your cluster probably has bigger problems), and the queries will get cancelled eventually anyways when ReportExecStatus() hits the timeout.
http://gerrit.cloudera.org:8080/#/c/16215/4/be/src/runtime/query-exec-mgr.cc@238
PS4, Line 238: if (cancellation_thread_pool_->GetQueueSize() + to_cancel.size()
Rather than ignoring all of the cancellations if there's too many, can we fill the queue up and ignore the rest?
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 4
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 24 Jul 2020 16:32:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 7: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 7
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 Jul 2020 21:39:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/exec-env.cc@554
PS3, Line 554: server->CancelQueriesOnFailedBackends(current_backend_set);
> I think this might be unnecessary on executor-only nodes?
Yeah for this and your other comment below, probably the cleanest thing is two just have two distinct callbacks, one that is registered if is_executor and calls CancelQueriesForFailedCoordinators, and one for is_coordinator that call CancelQueriesOnFailedBackends.
If both is_executor and is_coordinator is true you'll just register both callbacks, but ClusterMembershipMgr can handle that.
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/query-exec-mgr.cc
File be/src/runtime/query-exec-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/query-exec-mgr.cc@222
PS3, Line 222: // TODO: create cancellation task queue and working thread to run cancellation tasks
: // on a separate thread. If the queue is full, ignore the cancellations since we'll
: // be able to process them on the next heartbeat instead.
:
: for (auto& qs : to_cancel) {
: VLOG(1) << "CancelQueriesForFailedCoordinators(): cancel query " << qs->query_id();
: qs->Cancel();
: qs->is_coord_active_.Store(false);
: ReleaseQueryState(qs);
: }
> if you move this into impala-server.cc you can use the ImpalaServer::Cancel
I had suggested not to put this in ImpalaServer because its primarily the client interface (eg. beeswax and hs2) and is therefore really a coordinator component.
It's true that executor-only impalads will still have an ImpalaServer instance, but we might someday want to change that as we clean up the separation between executors and coordinators. On the other hand, ImpalaServer is probably so intertwined with everything else, and the benefit would be small enough, that we'll never actually do that.
It also doesn't seem like that much code duplication to put it here.
I don't really feel strongly about it, though.
http://gerrit.cloudera.org:8080/#/c/16215/3/tests/custom_cluster/test_restart_services.py
File tests/custom_cluster/test_restart_services.py:
http://gerrit.cloudera.org:8080/#/c/16215/3/tests/custom_cluster/test_restart_services.py@241
PS3, Line 241: def test_kill_coordinator(self):
This is an okay place for this test (obviously we have a lot of different test files and the distinction between them isn't always clear), but maybe instead put it test_process_failures.py instead?
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Jul 2020 19:25:00 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 5: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16215/5/be/src/runtime/query-exec-mgr.h
File be/src/runtime/query-exec-mgr.h:
http://gerrit.cloudera.org:8080/#/c/16215/5/be/src/runtime/query-exec-mgr.h@37
PS5, Line 37: absence
nit: absent
http://gerrit.cloudera.org:8080/#/c/16215/5/be/src/runtime/query-exec-mgr.cc
File be/src/runtime/query-exec-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/16215/5/be/src/runtime/query-exec-mgr.cc@61
PS5, Line 61: // queue up to the maximum limit and ignore the rest.
Could you also add: 'The ignored queries will get cancelled when they time out trying to send status reports.'
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 5
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Jul 2020 16:10:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16215/5/be/src/runtime/query-exec-mgr.h
File be/src/runtime/query-exec-mgr.h:
http://gerrit.cloudera.org:8080/#/c/16215/5/be/src/runtime/query-exec-mgr.h@37
PS5, Line 37: absence
> nit: absent
fixed it.
http://gerrit.cloudera.org:8080/#/c/16215/5/be/src/runtime/query-exec-mgr.cc
File be/src/runtime/query-exec-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/16215/5/be/src/runtime/query-exec-mgr.cc@61
PS5, Line 61: // queue up to the maximum limit and ignore the rest.
> Could you also add: 'The ignored queries will get cancelled when they time
fixed it
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 5
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Jul 2020 17:34:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
IMPALA-5746: Cancel all queries scheduled by failed coordinators
Executor registers the updating of cluster membership. When coordinators
are absence from the active cluster membership list, executer cancels
all the running fragments of the queries which are scheduled by the
inactive coordinators since the executer cannot send results back to
the inactive/failed coordinators. This makes executers quickly release
the resources allocated for those running fragments to be cancelled.
Testing:
- Added new test case TestProcessFailures::test_kill_coordinator
and ran the test case as following command:
./bin/impala-py.test tests/custom_cluster/test_process_failures.py\
::TestProcessFailures::test_kill_coordinator \
--exploration_strategy=exhaustive.
- Passed the core test.
Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/test-env.cc
M common/protobuf/control_service.proto
M tests/custom_cluster/test_process_failures.py
9 files changed, 194 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/16215/5
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 5
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
IMPALA-5746: Cancel all queries scheduled by failed coordinators
Executor registers the updating of cluster membership. When coordinators
are absence from the active cluster membership list, executer cancels
all the running fragments of the queries which are scheduled by the
inactive coordinators since the executer cannot send results back to
the inactive/failed coordinators. This makes executers quickly release
the resources allocated for those running fragments to be cancelled.
Testing:
- Added new test case TestProcessFailures::test_kill_coordinator
and ran the test case as following command:
./bin/impala-py.test tests/custom_cluster/test_process_failures.py\
::TestProcessFailures::test_kill_coordinator \
--exploration_strategy=exhaustive.
- Passed the core test.
Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/test-env.cc
M common/protobuf/control_service.proto
M tests/custom_cluster/test_process_failures.py
9 files changed, 195 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/16215/6
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 6
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
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/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
IMPALA-5746: Cancel all queries scheduled by failed coordinators
Executor registers the updating of cluster membership. When coordinators
are absence from the active cluster membership list, executer cancels
all the running fragments of the queries which are scheduled by the
inactive coordinators since the executer cannot send results back to
the inactive/failed coordinators. This makes executers quickly release
the resources allocated for those running fragments to be cancelled.
Testing:
- Added new test case TestProcessFailures::test_kill_coordinator
and ran the test case as following command:
./bin/impala-py.test tests/custom_cluster/test_process_failures.py\
::TestProcessFailures::test_kill_coordinator \
--exploration_strategy=exhaustive.
- Passed the core test.
Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Reviewed-on: http://gerrit.cloudera.org:8080/16215
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/test-env.cc
M common/protobuf/control_service.proto
M tests/custom_cluster/test_process_failures.py
9 files changed, 195 insertions(+), 14 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 8
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6729/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 6
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Jul 2020 18:05:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6705/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 5
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 24 Jul 2020 19:49:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16215/4/be/src/runtime/query-exec-mgr.h
File be/src/runtime/query-exec-mgr.h:
http://gerrit.cloudera.org:8080/#/c/16215/4/be/src/runtime/query-exec-mgr.h@38
PS4, Line 38: /// query execution.
> Could you update this comment to include:
Updated the comments as suggested.
http://gerrit.cloudera.org:8080/#/c/16215/4/be/src/runtime/query-exec-mgr.h@97
PS4, Line 97: boost::scoped_ptr
> We prefer to use std::unique_ptr in new code
fixed it.
http://gerrit.cloudera.org:8080/#/c/16215/4/be/src/runtime/query-exec-mgr.cc
File be/src/runtime/query-exec-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/16215/4/be/src/runtime/query-exec-mgr.cc@61
PS4, Line 61: retried on the next statestore heartbeat
> I don't think this is true (I see that you copied this comment from impala-
Right, fixed it.
http://gerrit.cloudera.org:8080/#/c/16215/4/be/src/runtime/query-exec-mgr.cc@238
PS4, Line 238: if (cancellation_thread_pool_->GetQueueSize() + to_cancel.size()
> Rather than ignoring all of the cancellations if there's too many, can we f
Right, we should fill the queue up to maximum capacity. Fixed.
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 4
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 24 Jul 2020 19:13:13 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16215/2/tests/custom_cluster/test_restart_services.py
File tests/custom_cluster/test_restart_services.py:
http://gerrit.cloudera.org:8080/#/c/16215/2/tests/custom_cluster/test_restart_services.py@239
PS2, Line 239: @
flake8: E303 too many blank lines (2)
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Sun, 19 Jul 2020 16:44:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6204/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 7
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 Jul 2020 16:35:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6644/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Sun, 19 Jul 2020 17:19:50 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/exec-env.cc@554
PS3, Line 554: server->CancelQueriesOnFailedBackends(current_backend_set);
> I was thinking to reuse the backend set and save a loop with one callback f
Yeah +1 to what Thomas said.
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/query-exec-mgr.cc
File be/src/runtime/query-exec-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/query-exec-mgr.cc@222
PS3, Line 222: // TODO: create cancellation task queue and working thread to run cancellation tasks
: // on a separate thread. If the queue is full, ignore the cancellations since we'll
: // be able to process them on the next heartbeat instead.
:
: for (auto& qs : to_cancel) {
: VLOG(1) << "CancelQueriesForFailedCoordinators(): cancel query " << qs->query_id();
: qs->Cancel();
: qs->is_coord_active_.Store(false);
: ReleaseQueryState(qs);
: }
> Will define a new thread pool owned by QueryExecMgr.
Yeah separate thread pool seems fine. Yeah, I'm fine with keeping this out of ImpalaServer.
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Jul 2020 18:38:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 7
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 Jul 2020 16:35:50 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5746: Cancel all queries scheduled by failed coordinators
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16215 )
Change subject: IMPALA-5746: Cancel all queries scheduled by failed coordinators
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/exec-env.cc@554
PS3, Line 554: server->CancelQueriesOnFailedBackends(current_backend_set);
I think this might be unnecessary on executor-only nodes?
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/exec-env.cc@560
PS3, Line 560: cluster_membership_mgr_->RegisterUpdateCallbackFn(
: [server](ClusterMembershipMgr::SnapshotPtr snapshot) {
: std::unordered_set<BackendIdPB> current_backend_set;
: for (const auto& it : snapshot->current_backends) {
: current_backend_set.insert(it.second.backend_id());
: }
: server->CancelQueriesOnFailedBackends(current_backend_set);
: });
not necessarily related to your change, but it would be nice to make this callback specific to coordinator nodes, because I think CancelQueriesOnFailedBackends only ever does anything on coordinator nodes.
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/query-exec-mgr.cc
File be/src/runtime/query-exec-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/16215/3/be/src/runtime/query-exec-mgr.cc@222
PS3, Line 222: // TODO: create cancellation task queue and working thread to run cancellation tasks
: // on a separate thread. If the queue is full, ignore the cancellations since we'll
: // be able to process them on the next heartbeat instead.
:
: for (auto& qs : to_cancel) {
: VLOG(1) << "CancelQueriesForFailedCoordinators(): cancel query " << qs->query_id();
: qs->Cancel();
: qs->is_coord_active_.Store(false);
: ReleaseQueryState(qs);
: }
if you move this into impala-server.cc you can use the ImpalaServer::CancelFromThreadPool. see ImpalaServer::CancelQueriesOnFailedBackends
--
To view, visit http://gerrit.cloudera.org:8080/16215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I918fcc27649d5d2bbe8b6ef47fbd9810ae5f57bd
Gerrit-Change-Number: 16215
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Jul 2020 18:44:55 +0000
Gerrit-HasComments: Yes