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

restore task after dm-workers restart #88

Merged
merged 24 commits into from
Mar 26, 2019
Merged

restore task after dm-workers restart #88

merged 24 commits into from
Mar 26, 2019

Conversation

IANTHEREAL
Copy link
Collaborator

@IANTHEREAL IANTHEREAL commented Mar 21, 2019

What problem does this PR solve?

What is changed and how it works?

store subtask in local disk of dm-worker.

In all request start/stop/update task, dm-worker would change meta firstly, then handle the request, we should explain it in document.

And I would modify ansible and add integrate test later

Check List

Tests

  • Unit test
  • integration test
  • Manual test
    • deploy a dm cluster
    • start a task
    • check dm worker meta file content
    • restart dm-worker
    • check whether task is running
    • add another task
    • check dm worker meta file content
    • stop one task
    • check dm worker meta file content
    • update task
    • check dm worker meta file content

@IANTHEREAL IANTHEREAL added status/PTAL This PR is ready for review. Add this label back after committing new changes type/enhancement Performance improvement or refactoring labels Mar 21, 2019
@IANTHEREAL IANTHEREAL changed the title Ian/restore task restore task after dm-workers restart Mar 21, 2019
@IANTHEREAL IANTHEREAL added status/WIP This PR is still work in progress and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Mar 21, 2019
@IANTHEREAL IANTHEREAL added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/WIP This PR is still work in progress labels Mar 21, 2019
checker/checker.go Outdated Show resolved Hide resolved
dm/worker/meta.go Show resolved Hide resolved
checker/checker.go Outdated Show resolved Hide resolved
return nil

_, err = c.DecryptPassword()
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

better return errors.Trace(err)

dm/worker/config.go Show resolved Hide resolved
dm/worker/config.go Outdated Show resolved Hide resolved
dm/worker/meta.go Show resolved Hide resolved
dm/worker/meta.go Show resolved Hide resolved
dm/worker/meta.go Outdated Show resolved Hide resolved

echo "[$(date)] <<<<<< START DM-WORKER on port $port, config: $conf >>>>>>"
cd $workdir
$binary -test.coverprofile="$TEST_DIR/cov.$TEST_NAME.worker.$port.out" DEVEL \
Copy link
Contributor

@amyangfei amyangfei Mar 23, 2019

Choose a reason for hiding this comment

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

It seems restart_dm_worker has quite similar logic to run_dm_worker script, is there any purpose to add this? In addition, the content of coverage file $TEST_DIR/cov.$TEST_NAME.worker.$port.out will be truncated here, we'd better to use append stream to this file(>>) or choose another file name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

remove mkdir and ln opeations, maybe we also add other operation in it later, how do you think about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

mkdir -p is reentrant, and we can also update ln to [ -f $workdir/bin/mydumper ] || ln -s $PWD/bin/mydumper $workdir/bin/mydumper to make it reentrant

amyangfei and others added 4 commits March 25, 2019 10:38
Co-Authored-By: GregoryIan <ArGregoryIan@gmail.com>
Co-Authored-By: GregoryIan <ArGregoryIan@gmail.com>
dm/worker/config.go Outdated Show resolved Hide resolved
dm/worker/meta.go Outdated Show resolved Hide resolved
dm/worker/meta.go Show resolved Hide resolved
dm/worker/meta.go Show resolved Hide resolved
dm/worker/meta_test.go Show resolved Hide resolved
dm/worker/server.go Show resolved Hide resolved
dm/worker/server.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@IANTHEREAL
Copy link
Collaborator Author

@csuzhangxc @amyangfei PTAL

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 Mar 25, 2019
Copy link
Contributor

@amyangfei amyangfei left a comment

Choose a reason for hiding this comment

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

LGTM

@amyangfei amyangfei added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Mar 25, 2019
@IANTHEREAL IANTHEREAL merged commit a11ce60 into pingcap:master Mar 26, 2019
@IANTHEREAL IANTHEREAL deleted the ian/restore-task branch March 26, 2019 00:57
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
status/LGT2 Two reviewers already commented LGTM, ready for merge type/enhancement Performance improvement or refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants