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

.*: fix bug that after execute pause-task the task may still running #644

Merged
merged 10 commits into from
Apr 30, 2020

Conversation

WangXiangUSTC
Copy link
Contributor

@WangXiangUSTC WangXiangUSTC commented Apr 29, 2020

What problem does this PR solve?

fix issue #637 and #629

when executing pause task the task is not real paused, and then execute resume task, maybe the old unit is still process, and then will cause some problem.

What is changed and how it works?

  1. add two stage pausing and resuming:
  • when pausing task, first the stage is pausing, and update stage to paused at the last.
  • when resuming task, first the stage is resuming, and update stage to running at the last.
  1. a minor fix, fetchResult should wait for the unit's process result.

Check List

Tests

  • Integration test

@WangXiangUSTC WangXiangUSTC added the status/WIP This PR is still work in progress label Apr 29, 2020
@WangXiangUSTC WangXiangUSTC marked this pull request as ready for review April 29, 2020 08:06
@WangXiangUSTC WangXiangUSTC added status/PTAL This PR is ready for review. Add this label back after committing new changes type/bug-fix Bug fix and removed status/WIP This PR is still work in progress labels Apr 29, 2020
@WangXiangUSTC WangXiangUSTC added this to the v2.0.0 beta.1 milestone Apr 29, 2020
@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

dm/unit/unit.go Outdated Show resolved Hide resolved
@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

@codecov
Copy link

codecov bot commented Apr 29, 2020

Codecov Report

Merging #644 into master will decrease coverage by 0.1091%.
The diff coverage is 57.1428%.

@@               Coverage Diff                @@
##             master       #644        +/-   ##
================================================
- Coverage   57.8327%   57.7236%   -0.1092%     
================================================
  Files           202        203         +1     
  Lines         20561      20515        -46     
================================================
- Hits          11891      11842        -49     
- Misses         7524       7526         +2     
- Partials       1146       1147         +1     

@@ -462,6 +472,7 @@ func (st *SubTask) Resume() error {
st.setStage(pb.Stage_Paused)
return err
} else if ctx.Err() != nil {
st.setStage(pb.Stage_Paused)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should not be set to pb.Stage_Paused, see #589 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I remove this and add a comment e34ec49

Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

Rest LGTM

@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

run_dm_ctl_with_retry $WORK_DIR "127.0.0.1:$MASTER_PORT" \
"resume-task test"\
"\"result\": true" 3
# resume twice
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a test to test stop-task twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add in ecd45e6

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/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Apr 30, 2020
@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

LGTM

@lichunzhu lichunzhu added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Apr 30, 2020
@WangXiangUSTC WangXiangUSTC merged commit 29df7b5 into master Apr 30, 2020
@WangXiangUSTC WangXiangUSTC deleted the xiang/fix_pause_resume branch April 30, 2020 09:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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