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.

Sign up for GitHub

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

Jump to bottom

store: add lock for runtime stats to fix panic caused by concurrent execution #18983

Merged
merged 7 commits into from
Aug 5, 2020
Merged

store: add lock for runtime stats to fix panic caused by concurrent execution #18983

merged 7 commits into from
Aug 5, 2020

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Aug 5, 2020 •
edited
Loading

Signed-off-by: crazycs520 crazycs520@gmail.com

What problem does this PR solve?

fix #18985

What is changed and how it works?

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit Test

Side effects

  • Performance regression
    • Consumes more CPU

Release note

  • Add lock for runtime stats to fix panic caused by concurrent execution.

@crazycs520
store: add lock to avoid concurrent panic
ad07408 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520 crazycs520 added the type/bugfix This PR fixes a bug. label Aug 5, 2020
@codecov
Copy link

codecov bot commented Aug 5, 2020 •
edited
Loading

Codecov Report

Merging #18983 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #18983   +/-   ##
===========================================
  Coverage   79.3862%   79.3862%           
===========================================
  Files           546        546           
  Lines        147964     147964           
===========================================
  Hits         117463     117463           
  Misses        21009      21009           
  Partials       9492       9492

@crazycs520
add unit test to detect race
b3e48b8 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520 crazycs520 requested a review from a team as a code owner August 5, 2020 03:07
@crazycs520 crazycs520 requested review from qw4990 and removed request for a team August 5, 2020 03:07
@crazycs520
Copy link
Contributor Author

/run-all-tests

@github-actions github-actions bot added the sig/execution SIG execution label Aug 5, 2020
@crazycs520
address comment
0d90722 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520
Merge branch 'fix-panix-map' of https://github.com/crazycs520/tidb in…
d6cc19c 
…to fix-panix-map
qw4990
qw4990 reviewed Aug 5, 2020
View reviewed changes
Copy link
Contributor

@qw4990 qw4990 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 Aug 5, 2020
@qw4990 qw4990 requested a review from wshwsh12 August 5, 2020 03:28
@crazycs520
Copy link
Contributor Author

/run-all-tests

bb7133
bb7133 approved these changes Aug 5, 2020
View reviewed changes
Copy link
Member

@bb7133 bb7133 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

@bb7133,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: execution(slack).

@crazycs520
Copy link
Contributor Author

/run-check_dev_2

@crazycs520
Copy link
Contributor Author

/run-unit-test

1 similar comment
@crazycs520
Copy link
Contributor Author

/run-unit-test

@crazycs520 crazycs520 merged commit 8976ffe into pingcap:master Aug 5, 2020
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 5, 2020
@crazycs520 @ti-srebot
cherry pick pingcap#18983 to release-4.0
4668405 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Aug 5, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #18991

ti-srebot added a commit that referenced this pull request Aug 5, 2020
@ti-srebot
store: add lock for runtime stats to fix panic caused by concurrent e…
ed27a9f 
…xecution (#18983) (#18991)

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qw4990 qw4990 qw4990 left review comments

@bb7133 bb7133 bb7133 approved these changes

@wshwsh12 wshwsh12 Awaiting requested review from wshwsh12

Assignees
No one assigned
Labels
component/executor sig/execution SIG execution status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

panic in mergeRegionRequestStats
4 participants
@crazycs520 @ti-srebot @bb7133 @qw4990