MPP: Kill mpp queries #23056

fzhedu · 2021-03-02T13:30:16Z

What problem does this PR solve?

support killing mpp queries via mysql client like kill tidb query 3;

besides, avoid goroutine leaking once one of mpp goroutines errors.

Issue Number: close pingcap/tiflash#1443

Problem Summary:

What is changed and how it works?

mysql> show processlist;
+------+------+-----------------+------+---------+------+------------+---------------------------------------------------------------------------+
| Id   | User | Host            | db   | Command | Time | State      | Info                                                                      |
+------+------+-----------------+------+---------+------+------------+---------------------------------------------------------------------------+
|    3 | root | 127.0.0.1:49218 | test | Query   |    3 | autocommit | explain analyze select * from lineitem,orders where l_orderkey=o_orderkey |
|    5 | root | 127.0.0.1:49247 | test | Query   |    0 | autocommit | show processlist                                                          |
+------+------+-----------------+------+---------+------+------------+---------------------------------------------------------------------------+
2 rows in set (0.00 sec)

mysql> kill tidb query 3;
Query OK, 0 rows affected (0.00 sec)

the working client:

 explain analyze select * from lineitem,orders where l_orderkey=o_orderkey;
ERROR 1317 (70100): Query execution was interrupted

Proposal: xxx

What's Changed:

How it Works:

Related changes

PR to update pingcap/docs/pingcap/docs-cn:

Check List

Tests

Manual test (add detailed scripts or steps below)

start another mysql client
show processlist;
kill tidb query xxx;

unit test (work with add ctx into mppctx ngaut/unistore#441)

Release note

kill mpp queries

store/copr/mpp.go

fzhedu · 2021-03-03T03:38:46Z

/run-check_dev

ichn-hu

Is there any test for this?

ichn-hu · 2021-03-05T11:17:10Z

store/copr/mpp.go

@@ -227,6 +229,25 @@ func (m *mppIterator) handleDispatchReq(ctx context.Context, bo *tikv.Backoffer,
 	m.establishMPPConns(bo, req, taskMeta)
 }

+// TODO: retry once failed?
+func (m *mppIterator) cancelMppTask(bo *tikv.Backoffer, meta *mpp.TaskMeta) {


should this function at least return a true or false on success or failure?

as comment in the NOTE.

ichn-hu · 2021-03-05T11:18:32Z

store/copr/mpp.go

-			m.sendToRespCh(&mppResponse{
-				err: errors.New(resp.Error.Msg),
-			})
+			// TODO


what does this TODO mean?

hanfei1991

Now that we have a mock mpp, why not write a unit test?

store/copr/mpp.go

fzhedu · 2021-03-09T14:15:55Z

Is there any test for this?

added.

store/copr/mpp.go

executor/tiflash_test.go

ti-chi-bot · 2021-03-12T08:08:40Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

hanfei1991
ichn-hu

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

fzhedu · 2021-03-12T08:09:11Z

/merge

ti-chi-bot · 2021-03-12T08:09:14Z

This pull request has been accepted and is ready to merge.

Commit hash: bba1eb9

hanfei1991 · 2021-03-12T08:09:21Z

/merge

ti-chi-bot · 2021-03-12T08:11:10Z

@fzhedu: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

fzhedu · 2021-03-12T08:33:34Z

/run-unit-test

fzhedu · 2021-03-12T09:19:21Z

/merge

ti-chi-bot · 2021-03-12T09:19:24Z

This pull request has been accepted and is ready to merge.

Commit hash: 5929ba4

fzhedu requested a review from a team as a code owner March 2, 2021 13:30

fzhedu requested review from XuHuaiyu and removed request for a team March 2, 2021 13:30

ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 2, 2021

fzhedu requested review from windtalker and hanfei1991 March 2, 2021 13:30

fzhedu added the type/new-feature label Mar 2, 2021

github-actions bot added the sig/execution SIG execution label Mar 2, 2021

fzhedu mentioned this pull request Mar 3, 2021

MPP: add CancelMPPTask for FlashService pingcap/tiflash#1505

Merged

windtalker reviewed Mar 3, 2021

View reviewed changes

store/copr/mpp.go Outdated Show resolved Hide resolved

fzhedu requested review from windtalker and ichn-hu March 3, 2021 06:45

ichn-hu requested changes Mar 5, 2021

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2021

hanfei1991 reviewed Mar 8, 2021

View reviewed changes

store/copr/mpp.go Outdated Show resolved Hide resolved

store/copr/mpp.go Outdated Show resolved Hide resolved

store/copr/mpp.go Outdated Show resolved Hide resolved

store/copr/mpp.go Outdated Show resolved Hide resolved

fzhedu force-pushed the killMpp branch from 6f88bd0 to f3af0d6 Compare March 8, 2021 09:48

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 8, 2021

fzhedu requested review from hanfei1991 and ichn-hu March 9, 2021 14:15

fzhedu force-pushed the killMpp branch from abaa843 to 6c4bd5f Compare March 10, 2021 01:35

fzhedu mentioned this pull request Mar 10, 2021

add ctx into mppctx ngaut/unistore#441

Merged

hanfei1991 reviewed Mar 10, 2021

View reviewed changes

store/copr/mpp.go Outdated Show resolved Hide resolved

hanfei1991 reviewed Mar 10, 2021

View reviewed changes

store/copr/mpp.go Outdated Show resolved Hide resolved

hanfei1991 reviewed Mar 10, 2021

View reviewed changes

store/copr/mpp.go Outdated Show resolved Hide resolved

hanfei1991 reviewed Mar 10, 2021

View reviewed changes

executor/tiflash_test.go Show resolved Hide resolved

fzhedu force-pushed the killMpp branch from f2896db to 333ec8a Compare March 11, 2021 08:49

fzhedu added 4 commits March 12, 2021 16:05

update go.mod

2deb8ea

add test for avoiding goroutine leak

50b6b88

update test

ff4026d

update test

bba1eb9

fzhedu force-pushed the killMpp branch from e24a364 to bba1eb9 Compare March 12, 2021 08:05

hanfei1991 approved these changes Mar 12, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 12, 2021

hanfei1991 self-assigned this Mar 12, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 12, 2021

fzhedu added 2 commits March 12, 2021 16:40

remove unnecessary leak test

0c82290

Merge branch 'master' of https://github.com/pingcap/tidb into killMpp

f44c5d8

fzhedu force-pushed the killMpp branch from 20b87d6 to f44c5d8 Compare March 12, 2021 08:42

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 12, 2021

Merge branch 'master' into killMpp

5929ba4

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 12, 2021

hanfei1991 merged commit 6b69883 into pingcap:master Mar 12, 2021

fzhedu mentioned this pull request Mar 15, 2021

test: add testleak after checking and testing #23324

Merged

ti-srebot mentioned this pull request Mar 18, 2021

test: add testleak after checking and testing (#23324) #23410

Merged

leiysky mentioned this pull request Mar 19, 2021

The aggregation query hangs when mpp is on pingcap/tiflash#1586

Closed

windtalker mentioned this pull request Mar 19, 2021

fix bug that mpp_fail test cost too much time / Fix test clustered index test cases pingcap/tiflash#1591

Merged

ti-srebot mentioned this pull request Mar 20, 2021

fix bug that mpp_fail test cost too much time / Fix test clustered index test cases (#1591) pingcap/tiflash#1600

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MPP: Kill mpp queries #23056

MPP: Kill mpp queries #23056

fzhedu commented Mar 2, 2021 •

edited

Loading

fzhedu commented Mar 3, 2021

ichn-hu left a comment

ichn-hu Mar 5, 2021

fzhedu Mar 8, 2021

ichn-hu Mar 5, 2021

fzhedu Mar 9, 2021

hanfei1991 left a comment

fzhedu commented Mar 9, 2021

ti-chi-bot commented Mar 12, 2021

fzhedu commented Mar 12, 2021

ti-chi-bot commented Mar 12, 2021

hanfei1991 commented Mar 12, 2021

ti-chi-bot commented Mar 12, 2021

fzhedu commented Mar 12, 2021

fzhedu commented Mar 12, 2021

ti-chi-bot commented Mar 12, 2021

MPP: Kill mpp queries #23056

MPP: Kill mpp queries #23056

Conversation

fzhedu commented Mar 2, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

fzhedu commented Mar 3, 2021

ichn-hu left a comment

Choose a reason for hiding this comment

ichn-hu Mar 5, 2021

Choose a reason for hiding this comment

fzhedu Mar 8, 2021

Choose a reason for hiding this comment

ichn-hu Mar 5, 2021

Choose a reason for hiding this comment

fzhedu Mar 9, 2021

Choose a reason for hiding this comment

hanfei1991 left a comment

Choose a reason for hiding this comment

fzhedu commented Mar 9, 2021

ti-chi-bot commented Mar 12, 2021

fzhedu commented Mar 12, 2021

ti-chi-bot commented Mar 12, 2021

hanfei1991 commented Mar 12, 2021

ti-chi-bot commented Mar 12, 2021

fzhedu commented Mar 12, 2021

fzhedu commented Mar 12, 2021

ti-chi-bot commented Mar 12, 2021

fzhedu commented Mar 2, 2021 •

edited

Loading