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

master: only fetch necessary DM-worker's config #316

Merged
merged 4 commits into from
Oct 17, 2019
Merged

master: only fetch necessary DM-worker's config #316

merged 4 commits into from
Oct 17, 2019

Conversation

csuzhangxc
Copy link
Member

@csuzhangxc csuzhangxc commented Oct 16, 2019 •
edited
Loading

What problem does this PR solve?

The previous version of DM-master always fetches all DM-workers' config when starting a task.

When having any un-accessible DM-worker, then we can't start any task.

What is changed and how it works?

Change to only fetches necessary DM-workers' config

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
    • start task with un-accessible DM-worker

Related changes

  • Need to cherry-pick to the release branch
  • Need to be included in the release note

@csuzhangxc csuzhangxc added priority/normal Minor change, requires approval from ≥1 primary reviewer status/PTAL This PR is ready for review. Add this label back after committing new changes type/bug-fix Bug fix priority/release-blocker This PR blocks a release. Please review it ASAP. 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 needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Oct 16, 2019
@csuzhangxc
Copy link
Member Author

/run-all-tests

@codecov
Copy link

codecov bot commented Oct 16, 2019 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master       #316   +/-   ##
===========================================
  Coverage   60.6871%   60.6871%           
===========================================
  Files           134        134           
  Lines         14845      14845           
===========================================
  Hits           9009       9009           
  Misses         4966       4966           
  Partials        870        870

@csuzhangxc
Copy link
Member Author

@WangXiangUSTC @amyangfei PTAL

@WangXiangUSTC
Copy link
Contributor

LGTM, and it is better to add a test

@WangXiangUSTC WangXiangUSTC 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 Oct 17, 2019
@csuzhangxc
Copy link
Member Author

LGTM, and it is better to add a test

@WangXiangUSTC I modify start_task test case in e13abaf, PTAL again.

@WangXiangUSTC
Copy link
Contributor

LGTM

amyangfei
amyangfei approved these changes Oct 17, 2019
View reviewed changes
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

@csuzhangxc csuzhangxc merged commit 39d8f5d into pingcap:master Oct 17, 2019
@amyangfei amyangfei added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Oct 17, 2019
@csuzhangxc csuzhangxc deleted the fetch-worker-config branch October 17, 2019 06:44
@sre-bot sre-bot mentioned this pull request Oct 17, 2019
Merged
@sre-bot
Copy link

sre-bot commented Oct 17, 2019

cherry pick to release-1.0 in PR #319

@sre-bot sre-bot 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 Oct 17, 2019
amyangfei pushed a commit that referenced this pull request Oct 17, 2019
@sre-bot @amyangfei
master: only fetch necessary DM-worker's config (#316) (#319)
50c4a14
@csuzhangxc csuzhangxc added already-update-release-note The release note is updated. Add this label once the release note is updated and removed needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Nov 1, 2019
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
@csuzhangxc
master: only fetch necessary DM-worker's config (pingcap#316)
aa0ead9
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@amyangfei amyangfei amyangfei approved these changes

Assignees
No one assigned
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 already-update-release-note The release note is updated. Add this label once the release note is updated priority/normal Minor change, requires approval from ≥1 primary reviewer priority/release-blocker This PR blocks a release. Please review it ASAP. status/LGT2 Two reviewers already commented LGTM, ready for merge type/bug-fix Bug fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@csuzhangxc @WangXiangUSTC @sre-bot @amyangfei