fix a bug that ExchangeReceiver can't be canceled #4441

bestwoody · 2022-03-25T09:28:43Z

What problem does this PR solve?

Issue Number: close #4432

Problem Summary:

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

Signed-off-by: bestwoody <[email protected]>

…ugfix_cancel_recv

Signed-off-by: bestwoody <[email protected]>

ti-chi-bot · 2022-03-25T09:28:44Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

fuzhe1989
yibin87

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 submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

windtalker

LGTM

fuzhe1989 · 2022-03-25T09:42:46Z

dbms/src/Flash/Mpp/ExchangeReceiver.cpp

+    if (!setEndState(ExchangeReceiverState::CANCELED))
+        return;


Suggested change

if (!setEndState(ExchangeReceiverState::CANCELED))

return;

setEndState(ExchangeReceiverState::CANCELED);

it's ok for finishing a queue twice.

fuzhe1989 · 2022-03-25T09:43:28Z

dbms/src/Flash/Mpp/ExchangeReceiver.cpp

 {
+    assert(new_state == ExchangeReceiverState::CANCELED || new_state == ExchangeReceiverState::CLOSED);


please use RUNTIME_ASSERT.

experimental , we can updated in future

Signed-off-by: bestwoody <[email protected]>

bestwoody · 2022-03-25T11:34:22Z

/merge

ti-chi-bot · 2022-03-25T11:34:22Z

@bestwoody: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and 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.

If you have any questions about the PR merge process, please refer to pr process.

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.

ti-chi-bot · 2022-03-25T11:34:28Z

@bestwoody: /merge in this pull request requires 2 approval(s).

In response to this:

/merge

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.

bestwoody · 2022-03-25T11:56:20Z

/rebuild

bestwoody · 2022-03-25T12:21:04Z

/rebuild

fzhedu

/LGTM

fzhedu · 2022-03-25T12:29:26Z

/merge

ti-chi-bot · 2022-03-25T12:29:26Z

@fzhedu: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and 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.

If you have any questions about the PR merge process, please refer to pr process.

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.

bestwoody · 2022-03-25T13:14:13Z

/merge

bestwoody · 2022-03-25T13:14:47Z

/rebuild

bestwoody · 2022-03-25T13:32:23Z

/run-all-tests

bestwoody · 2022-03-25T13:35:06Z

/run-all-tests

bestwoody · 2022-03-25T13:35:25Z

/merge

ti-chi-bot · 2022-03-25T13:35:26Z

@bestwoody: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and 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.

If you have any questions about the PR merge process, please refer to pr process.

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.

sre-bot · 2022-03-25T13:49:57Z

Coverage for changed files

Filename                                   Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
DataStreams/TiRemoteBlockInputStream.h         115               115     0.00%          15                15     0.00%         150               150     0.00%          60                60     0.00%
Flash/Coprocessor/CoprocessorReader.h           49                49     0.00%          11                11     0.00%          80                80     0.00%          32                32     0.00%
Flash/Coprocessor/DAGContext.cpp                91                67    26.37%          25                17    32.00%         206               169    17.96%          64                54    15.62%
Flash/Coprocessor/DAGContext.h                  31                13    58.06%          24                12    50.00%          72                30    58.33%           8                 3    62.50%
Flash/Mpp/ExchangeReceiver.cpp                 308               308     0.00%          32                32     0.00%         459               459     0.00%         192               192     0.00%
Flash/Mpp/ExchangeReceiver.h                     9                 9     0.00%           7                 7     0.00%          15                15     0.00%           2                 2     0.00%
Flash/Mpp/MPPTask.cpp                          417               417     0.00%          21                21     0.00%         350               350     0.00%         190               190     0.00%
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                         1020               978     4.12%         135               115    14.81%        1332              1253     5.93%         548               533     2.74%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
17049      9575             43.84%    191774  96996        49.42%

full coverage report (for internal network access only)

ti-chi-bot · 2022-03-25T14:48:57Z

In response to a cherrypick label: new pull request created: #4445.

close #4432

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2022-03-31T07:35:17Z

In response to a cherrypick label: new pull request created: #4532.

This reverts commit 25e2a26.

* This is an automated cherry-pick of #4441 Signed-off-by: ti-chi-bot <[email protected]> * Revert "This is an automated cherry-pick of #4441" This reverts commit 25e2a26. * cherry pick

* This is an automated cherry-pick of #4441 Signed-off-by: ti-chi-bot <[email protected]> * Revert "This is an automated cherry-pick of #4441" This reverts commit 25e2a26. * cherry pick * format * pass code stlye check Co-authored-by: bestwoody <[email protected]> Co-authored-by: bestwoody <[email protected]>

yibin87

Ignore this, just a mistake.

bestwoody and others added 7 commits March 25, 2022 14:06

close receiver early

c14a5e9

Signed-off-by: bestwoody <[email protected]>

Merge branch 'master' into bugfix_cancel_recv

8430a86

update

431a8c6

Signed-off-by: bestwoody <[email protected]>

Merge branch 'bugfix_cancel_recv' of github.com:bestwoody/tics into b…

5ee78b2

…ugfix_cancel_recv

update

382b3b4

Signed-off-by: bestwoody <[email protected]>

add state check of exchgRecv

2335e40

Signed-off-by: bestwoody <[email protected]>

refine code

73280c1

ti-chi-bot added needs-cherry-pick-release-6.0 Type: Need cherry pick to release-6.0 release-note-none Denotes a PR that doesn't merit a release note. labels Mar 25, 2022

bestwoody requested review from fuzhe1989 and windtalker March 25, 2022 09:28

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

bestwoody requested a review from fzhedu March 25, 2022 09:28

bestwoody mentioned this pull request Mar 25, 2022

fix a bug that ExchangeReceiver can't be canceled #4436

Closed

12 tasks

windtalker approved these changes Mar 25, 2022

View reviewed changes

fuzhe1989 reviewed Mar 25, 2022

View reviewed changes

bestwoody added 3 commits March 25, 2022 17:47

use runtime assert instead

0f8df53

Signed-off-by: bestwoody <[email protected]>

revert runtime_assert

8f161d3

slim

13635f9

Signed-off-by: bestwoody <[email protected]>

bestwoody requested a review from yibin87 March 25, 2022 11:01

fuzhe1989 approved these changes Mar 25, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 25, 2022

bestwoody added the status/LGT2 Indicates that a PR has LGTM 2. label Mar 25, 2022

ti-chi-bot removed the status/LGT2 Indicates that a PR has LGTM 2. label Mar 25, 2022

bestwoody added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 25, 2022

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

fzhedu approved these changes Mar 25, 2022

View reviewed changes

bestwoody merged commit a3f804e into master Mar 25, 2022

ti-chi-bot mentioned this pull request Mar 25, 2022

fix a bug that ExchangeReceiver can't be canceled (#4441) #4445

Merged

12 tasks

bestwoody deleted the bugfix_cancel_recv branch March 25, 2022 14:59

ti-chi-bot added a commit that referenced this pull request Mar 26, 2022

fix a bug that ExchangeReceiver can't be canceled (#4441) (#4445)

81025ae

close #4432

bestwoody added the needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. label Mar 31, 2022

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request Mar 31, 2022

This is an automated cherry-pick of pingcap#4441

25e2a26

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Mar 31, 2022

fix a bug that ExchangeReceiver can't be canceled (#4441) #4532

Merged

12 tasks

bestwoody added a commit to ti-chi-bot/tiflash that referenced this pull request Apr 14, 2022

Revert "This is an automated cherry-pick of pingcap#4441"

61c06fb

This reverts commit 25e2a26.

This was referenced Apr 14, 2022

fix a bug that ExchangeReceiver can't be canceled (#4441) #4663

Closed

fix a bug that ExchangeReceiver can't be canceled (#4441) #4664

Merged

yibin87 reviewed Apr 15, 2022

View reviewed changes

windtalker mentioned this pull request Jun 9, 2022

Refine error handling/cancel in TiFlash MPP system #5095

Closed

windtalker mentioned this pull request Jun 24, 2022

add MPPTask::handleError() #5202

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix a bug that ExchangeReceiver can't be canceled #4441

fix a bug that ExchangeReceiver can't be canceled #4441

bestwoody commented Mar 25, 2022

ti-chi-bot commented Mar 25, 2022 •

edited

Loading

windtalker left a comment

fuzhe1989 Mar 25, 2022

bestwoody Mar 25, 2022

fuzhe1989 Mar 25, 2022

bestwoody Mar 25, 2022

bestwoody Mar 25, 2022

bestwoody commented Mar 25, 2022

ti-chi-bot commented Mar 25, 2022

ti-chi-bot commented Mar 25, 2022

bestwoody commented Mar 25, 2022

bestwoody commented Mar 25, 2022

fzhedu left a comment

fzhedu commented Mar 25, 2022

ti-chi-bot commented Mar 25, 2022

bestwoody commented Mar 25, 2022

bestwoody commented Mar 25, 2022

bestwoody commented Mar 25, 2022

bestwoody commented Mar 25, 2022

bestwoody commented Mar 25, 2022

ti-chi-bot commented Mar 25, 2022

sre-bot commented Mar 25, 2022

ti-chi-bot commented Mar 25, 2022

ti-chi-bot commented Mar 31, 2022

yibin87 left a comment •

edited

Loading

	if (!setEndState(ExchangeReceiverState::CANCELED))
	return;
	setEndState(ExchangeReceiverState::CANCELED);

		{
		assert(new_state == ExchangeReceiverState::CANCELED \|\| new_state == ExchangeReceiverState::CLOSED);

fix a bug that ExchangeReceiver can't be canceled #4441

fix a bug that ExchangeReceiver can't be canceled #4441

Conversation

bestwoody commented Mar 25, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Mar 25, 2022 • edited Loading

windtalker left a comment

Choose a reason for hiding this comment

fuzhe1989 Mar 25, 2022

Choose a reason for hiding this comment

bestwoody Mar 25, 2022

Choose a reason for hiding this comment

fuzhe1989 Mar 25, 2022

Choose a reason for hiding this comment

bestwoody Mar 25, 2022

Choose a reason for hiding this comment

bestwoody Mar 25, 2022

Choose a reason for hiding this comment

bestwoody commented Mar 25, 2022

ti-chi-bot commented Mar 25, 2022

ti-chi-bot commented Mar 25, 2022

bestwoody commented Mar 25, 2022

bestwoody commented Mar 25, 2022

fzhedu left a comment

Choose a reason for hiding this comment

fzhedu commented Mar 25, 2022

ti-chi-bot commented Mar 25, 2022

bestwoody commented Mar 25, 2022

bestwoody commented Mar 25, 2022

bestwoody commented Mar 25, 2022

bestwoody commented Mar 25, 2022

bestwoody commented Mar 25, 2022

ti-chi-bot commented Mar 25, 2022

sre-bot commented Mar 25, 2022

Coverage for changed files

Coverage summary

ti-chi-bot commented Mar 25, 2022

ti-chi-bot commented Mar 31, 2022

yibin87 left a comment • edited Loading

Choose a reason for hiding this comment

ti-chi-bot commented Mar 25, 2022 •

edited

Loading

yibin87 left a comment •

edited

Loading