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

executor: fix panic when execute change pump state #11730

Merged
merged 9 commits into from
Aug 22, 2019

Conversation

WangXiangUSTC
Copy link
Contributor

What problem does this PR solve?

when execute sql like change pump to node_state ='paused' for node_id 'pump1', tidb will return error connection running loop panic

What is changed and how it works?

fix it

Check List

Tests

  • Manual test (exec sql)

Related changes

  • Need to cherry-pick to the release branch

@codecov
Copy link

codecov bot commented Aug 13, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #11730   +/-   ##
===========================================
  Coverage   81.5427%   81.5427%           
===========================================
  Files           435        435           
  Lines         94467      94467           
===========================================
  Hits          77031      77031           
  Misses        11947      11947           
  Partials       5489       5489

sessVars := base.ctx.GetSessionVars()
if atomic.CompareAndSwapUint32(&sessVars.Killed, 1, 0) {
return ErrQueryInterrupted
if base.ctx != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

When would base.ctx be nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe some other Executor forget to initial baseExecutor just like ChangeExec

Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we make it an error to leave baseExecutor out? It seems if it's nil then we are introducing something that cannot be interrupted by KILL here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed, PTAL again

Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a test for this case?

@shenli
Copy link
Member

shenli commented Aug 18, 2019

Please add a unit test case.

@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

@WangXiangUSTC
Copy link
Contributor Author

@shenli @suzaku add a unit test, PTAL again

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.

LGTM

@WangXiangUSTC WangXiangUSTC added status/LGT1 Indicates that a PR has LGTM 1. and removed status/PTAL labels Aug 21, 2019
Copy link
Contributor

@suzaku suzaku left a comment

Choose a reason for hiding this comment

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

LGTM

@WangXiangUSTC WangXiangUSTC added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 22, 2019
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp jackysp added the status/can-merge Indicates a PR has been approved by a committer. label Aug 22, 2019
@WangXiangUSTC WangXiangUSTC added needs-cherry-pick-3.0 and removed status/can-merge Indicates a PR has been approved by a committer. labels Aug 22, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Aug 22, 2019

/run-all-tests

@jackysp
Copy link
Member

jackysp commented Aug 22, 2019

Please fix CI, @WangXiangUSTC .

@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

@zz-jason zz-jason merged commit 8b679ed into pingcap:master Aug 22, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Aug 22, 2019

cherry pick to release-2.1 failed

@WangXiangUSTC WangXiangUSTC deleted the xiang/fix_panic branch August 22, 2019 05:45
@sre-bot
Copy link
Contributor

sre-bot commented Aug 22, 2019

cherry pick to release-3.0 failed

@sre-bot
Copy link
Contributor

sre-bot commented Apr 7, 2020

It seems that, not for sure, we failed to cherry-pick this commit to release-3.0. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @WangXiangUSTC PTAL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/tools status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants