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

dm-master: embed etcd in DM-master #344

Merged
merged 16 commits into from
Nov 12, 2019

Conversation

csuzhangxc
Copy link
Member

@csuzhangxc csuzhangxc commented Nov 4, 2019

What problem does this PR solve?

embed etcd in DM-master.

What is changed and how it works?

  • add some config items in DM-master which needed by etcd
    • name: human-readable name for this DM-master member (default 'dm-master-${hostname}')
    • data-dir: path to the data directory (default 'default.${name}')
    • initial-cluster: initial cluster configuration for bootstrapping, e,g. 'dm-master=http://127.0.0.1:8291'
    • peer-urls: URLs for peer traffic (default 'http://127.0.0.1:8291')
    • advertise-peer-urls: advertise URLs for peer traffic (default '${peer-urls}')
  • register gRPC API server, HTTP API server and status&debug HTTP server into embed etcd server.

NOTE: we use master-addr to generate listen-client-urls and advertise-client-urls now.

to reviewers:

  • previously DM-master listening 8261 for clients request, now another port is needed for peers request. I use 8291 as the default peer port, do you think is it OK?
  • this PR not support join a member to a existing cluster, and I'll add it in late PR
  • this PR only embed etcd into DM-master, no leader election exists yet, and I'll add it in late PR
  • this PR not handle etcd's log, and I'll handle it after ectd in tidb and tidb-tool upgraded

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported variable/fields change
  • Has persistent data change

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation
  • Need to update the dm/dm-ansible
  • Need to be included in the release note

@csuzhangxc csuzhangxc added priority/important Major change, requires approval from ≥2 primary reviewers needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated status/WIP This PR is still work in progress type/feature New feature type/feature-request This issue is a feature request needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated should-update-dm-ansible labels Nov 4, 2019
@codecov
Copy link

codecov bot commented Nov 4, 2019

Codecov Report

Merging #344 into master will decrease coverage by 0.0026%.
The diff coverage is 85.4961%.

@@               Coverage Diff                @@
##             master       #344        +/-   ##
================================================
- Coverage   57.6502%   57.6476%   -0.0027%     
================================================
  Files           159        159                
  Lines         16019      16018         -1     
================================================
- Hits           9235       9234         -1     
  Misses         5884       5884                
  Partials        900        900

@csuzhangxc csuzhangxc 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 Nov 6, 2019
@csuzhangxc csuzhangxc requested review from amyangfei and WangXiangUSTC and removed request for amyangfei November 6, 2019 02:41
@csuzhangxc
Copy link
Member Author

@WangXiangUSTC @amyangfei PTAL

@csuzhangxc
Copy link
Member Author

/run-all-tests

1 similar comment
@csuzhangxc
Copy link
Member Author

/run-all-tests

dm/master/config.go Show resolved Hide resolved
dm/master/config.go Outdated Show resolved Hide resolved
dm/master/config.go Outdated Show resolved Hide resolved
dm/master/config.go Outdated Show resolved Hide resolved
dm/master/config.go Show resolved Hide resolved
@WangXiangUSTC
Copy link
Contributor

previously DM-master listening 8261 for clients request, now another port is needed for peers request. I use 8269 as the default peer port, do you think is it OK?

some users deploy dm-master and more than one dm-worker in one server, perhaps they use port like 8261, 8262, 8263 ...
how about use another port?

@csuzhangxc
Copy link
Member Author

/run-all-tests

@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 Nov 12, 2019
@WangXiangUSTC
Copy link
Contributor

LGTM

@csuzhangxc
Copy link
Member Author

@amyangfei PTAL. we are adding another integration test case Integration Test import_gorout…, and it should be failed in this PR, that's not a problem.

dm/master/etcd.go Outdated Show resolved Hide resolved
dm/master/server.go Outdated Show resolved Hide resolved
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 Nov 12, 2019
@csuzhangxc
Copy link
Member Author

/run-all-tests

3 similar comments
@csuzhangxc
Copy link
Member Author

/run-all-tests

@csuzhangxc
Copy link
Member Author

/run-all-tests

@csuzhangxc
Copy link
Member Author

/run-all-tests

@csuzhangxc csuzhangxc merged commit 41275db into pingcap:master Nov 12, 2019
@csuzhangxc csuzhangxc removed the type/feature-request This issue is a feature request label Nov 12, 2019
@csuzhangxc csuzhangxc deleted the dm-master-owner branch November 12, 2019 09:35
@csuzhangxc csuzhangxc added the type/compatibility Backward compatibility will be broken after this PR merged label Nov 20, 2019
@csuzhangxc csuzhangxc mentioned this pull request Dec 6, 2019
23 tasks
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
@csuzhangxc csuzhangxc removed needs-update-dm-ansible needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Jun 17, 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/compatibility Backward compatibility will be broken after this PR merged type/feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants