Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

worker/: fix worker stuck in unitTransWaitCondition #589

Merged
merged 10 commits into from
Apr 20, 2020

Conversation

lichunzhu
Copy link
Contributor

What problem does this PR solve?

#576
When subtask is started, it will use unitTransWaitCondition to wait for relay log to catch up. But if this task is paused or stopped, we should quit as fast as we can instead of keeping waiting.

What is changed and how it works?

  1. Use st.currCtx to control wait of unitTransWaitCondition.
  2. Use parent context st.ctx and son context st.currCtx to make sure task can be shut down.

Check List

Tests

  • Unit test
  • Integration test

Related changes

  • Need to cherry-pick to the release branch

@lichunzhu lichunzhu added priority/normal Minor change, requires approval from ≥1 primary reviewer type/bug-fix Bug fix needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 labels Apr 9, 2020
@codecov
Copy link

codecov bot commented Apr 9, 2020

Codecov Report

Merging #589 into master will increase coverage by 0.3466%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master       #589        +/-   ##
================================================
+ Coverage   57.8518%   58.1985%   +0.3466%     
================================================
  Files           201        201                
  Lines         20371      20461        +90     
================================================
+ Hits          11785      11908       +123     
+ Misses         7455       7417        -38     
- Partials       1131       1136         +5     

if err != nil {
st.l.Error("wait condition", log.ShortError(err))
st.fail(err)
return
} else if ctx.Err() != nil {
return
Copy link
Member

Choose a reason for hiding this comment

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

Will the stage keep as Stage_Running?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ctx.Err() != nil means this context is canceled in other go routine, that go routine will change the stage.

Copy link
Member

Choose a reason for hiding this comment

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

So, how about checking the stage in the unit test cases?

dm/worker/subtask.go Show resolved Hide resolved
dm/worker/subtask_test.go Outdated Show resolved Hide resolved
dm/worker/subtask_test.go Outdated Show resolved Hide resolved
@csuzhangxc
Copy link
Member

/run-all-tests

Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

LGTM

@csuzhangxc csuzhangxc added the status/LGT1 One reviewer already commented LGTM label Apr 20, 2020
Copy link
Contributor

@WangXiangUSTC WangXiangUSTC 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 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Apr 20, 2020
@lichunzhu lichunzhu merged commit 59fc227 into pingcap:master Apr 20, 2020
@lichunzhu lichunzhu deleted the czli/fixUnitWait branch April 20, 2020 09:12
@sre-bot
Copy link

sre-bot commented Apr 20, 2020

cherry pick to release-1.0 failed

@csuzhangxc
Copy link
Member

@lichunzhu please do not forget to cherry-pick it manually.

@lichunzhu
Copy link
Contributor Author

@lichunzhu please do not forget to cherry-pick it manually.

@csuzhangxc OK

lichunzhu added a commit to lichunzhu/dm that referenced this pull request Apr 21, 2020
@csuzhangxc csuzhangxc added already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked and removed needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 labels Jun 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT2 Two reviewers already commented LGTM, ready for merge type/bug-fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants