Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Mar 19, 2021

Conversation

windtalker
Copy link
Contributor

@windtalker windtalker commented Mar 19, 2021

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:

After pingcap/tidb#23056, fullstack test mpp_fail takes too much time(more than 10 minutes), which is expected.

What is changed and how it works?

Proposal: xxx

What's Changed:

How it Works:

The root cause is after pingcap/tidb#23056, TiDB will send kill command to kill all the mpp tasks for a specified query, and currently implementation of cancelling a mpp task will call writeErrToAllTunnel to write error message to other mpp tasks, since all the mpp tasks are canceled at the same time, writeErrToAllTunnel might hangs for a little time(write error message until tunnel is timeout), so the test execution time is much more than expected. In this pr, when canceling a mpp task, it just call closeAllTunnel to close the tunnel, which only write error message to other mpp task if the tunnel is already connected.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

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

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM
  • Breaking backward compatibility

Release note

  • No release note

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 19, 2021
@leiysky
Copy link
Contributor

leiysky commented Mar 19, 2021

Will this PR fix the problem that DispatchMPPTasks failed will make TiDB wait for a long time to receive the error?

Related to #1586 (comment)

@windtalker
Copy link
Contributor Author

/run-all-tests

@windtalker
Copy link
Contributor Author

/run-all-tests

@windtalker windtalker merged commit 891466e into pingcap:master Mar 19, 2021
@JaySon-Huang
Copy link
Contributor

JaySon-Huang commented Mar 20, 2021

Mark that this PR also fixes the test cases of clustered index. pingcap/tidb#23270

@ti-srebot
Copy link
Collaborator

cherry pick to release-5.0 in PR #1600

JaySon-Huang pushed a commit that referenced this pull request Mar 20, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

Co-authored-by: xufei <xufei@pingcap.com>
@JaySon-Huang JaySon-Huang changed the title fix bug that mpp_fail test cost too much time fix bug that mpp_fail test cost too much time / Fix test clustered index test cases Mar 20, 2021
@windtalker
Copy link
Contributor Author

Will this PR fix the problem that DispatchMPPTasks failed will make TiDB wait for a long time to receive the error?

Related to #1586 (comment)

Yes, but this pr still have a small chances that TiFlash server will crash when meet some mpp errors, #1577 fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-cherry-pick-release-5.0 PR which needs to be cherry-picked to release-5.0 status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants