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 cop task runtime information is wrong in the concurrent executor (#19849) #19947

Merged
merged 7 commits into from
Oct 19, 2020

Conversation

ti-srebot
Copy link
Contributor

@ti-srebot ti-srebot commented Sep 11, 2020

cherry-pick #19849 to release-4.0


What problem does this PR solve?

close #19847

What is changed and how it works?

In some case such as index join, 1 plan maybe use multiple executor for concurrency, so, 1 plan ID maybe have multiple executor runtime stats.

This PR use an array to record executor runtime stats and merge them when query the executor runtime stats.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Manual test

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM

Release note

  • Fix cop task runtime information is wrong in the concurrent executor.

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ci failed

Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot
Copy link
Contributor Author

@AilinKid, Thanks for your review, however we are sorry that your vote won't be count.

@zz-jason
Copy link
Member

zz-jason commented Oct 3, 2020

@crazycs520 seems the cherry-pick is incorrect: Files changed (0)

@crazycs520
Copy link
Contributor

wait #20199 merge first.

@crazycs520
Copy link
Contributor

Wait #20430 merge first.

@crazycs520
Copy link
Contributor

/run-all-tests

@crazycs520
Copy link
Contributor

/run-unit-test

Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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

@tangenta tangenta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 19, 2020
@ti-srebot ti-srebot added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Oct 19, 2020
@crazycs520
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor Author

Sorry @crazycs520, this branch cannot be merged without an approval of release maintainers

@crazycs520
Copy link
Contributor

@SunRunAway, Could you help to merge?

@SunRunAway SunRunAway merged commit 34b70e1 into pingcap:release-4.0 Oct 19, 2020
@SunRunAway SunRunAway deleted the release-4.0-bada2801ac1f branch October 19, 2020 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants