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

worker: fix panic in handle task #225

Merged
merged 3 commits into from
Aug 6, 2019

Conversation

amyangfei
Copy link
Contributor

What problem does this PR solve?

Fix a panic bug in dm task hander
We can reproduce the panic with steps as follows:

  1. setup a DM cluster, start a task
  2. stop one of DM-worker, delete the dm_worker_meta directory of this node
  3. start this DM-worker, send stop-task, pause-task or resume-task from DM-master

What is changed and how it works?

If task meta is not stored in DM-worker's memory when we want to use it, log a warning message instead of using it.

Check List

Tests

  • Unit test

Code changes

Side effects

Related changes

@amyangfei amyangfei added priority/normal Minor change, requires approval from ≥1 primary reviewer type/bug-fix Bug fix labels Aug 5, 2019
@amyangfei
Copy link
Contributor Author

/run-all-tests

1 similar comment
@amyangfei
Copy link
Contributor Author

/run-all-tests

@codecov
Copy link

codecov bot commented Aug 5, 2019

Codecov Report

Merging #225 into master will decrease coverage by 0.0865%.
The diff coverage is 80%.

@@               Coverage Diff               @@
##            master       #225        +/-   ##
===============================================
- Coverage   59.155%   59.0685%   -0.0866%     
===============================================
  Files          125        123         -2     
  Lines        14320      14214       -106     
===============================================
- Hits          8471       8396        -75     
+ Misses        4996       4960        -36     
- Partials       853        858         +5

@amyangfei amyangfei added the status/PTAL This PR is ready for review. Add this label back after committing new changes label Aug 5, 2019
@IANTHEREAL
Copy link
Collaborator

Good Job! LGTM

@IANTHEREAL IANTHEREAL added priority/important Major change, requires approval from ≥2 primary reviewers status/LGT1 One reviewer already commented LGTM and removed priority/normal Minor change, requires approval from ≥1 primary reviewer labels Aug 5, 2019
failpoint.Inject("handleTaskInternal", func(val failpoint.Value) {
if milliseconds, ok := val.(int); ok {
handleTaskInterval = time.Duration(milliseconds) * time.Millisecond
w.l.Info("set handleTaskInterval", zap.Int("value", milliseconds))
Copy link
Member

Choose a reason for hiding this comment

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

log a field with failpoint key like other failpoint inject?

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 status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Aug 6, 2019
@amyangfei amyangfei merged commit f1f0f6b into pingcap:master Aug 6, 2019
@amyangfei amyangfei removed the status/PTAL This PR is ready for review. Add this label back after committing new changes label Aug 6, 2019
@amyangfei amyangfei deleted the refine-task-handler branch August 6, 2019 07:37
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority/important Major change, requires approval from ≥2 primary reviewers 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.

3 participants