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

worker: use random value as server-id if server-id is not set #329

Merged
merged 32 commits into from
Oct 25, 2019

Conversation

WangXiangUSTC
Copy link
Contributor

@WangXiangUSTC WangXiangUSTC commented Oct 21, 2019

What problem does this PR solve?

use random value as server-id if server-id is not set

What is changed and how it works?

if server id is not set, generate a random value for it

Check List

Tests

  • Unit test
  • Integration test

Related changes

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

@WangXiangUSTC WangXiangUSTC added 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-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 status/PTAL This PR is ready for review. Add this label back after committing new changes labels Oct 21, 2019
@codecov
Copy link

codecov bot commented Oct 21, 2019

Codecov Report

Merging #329 into master will decrease coverage by 0.0182%.
The diff coverage is 48.9583%.

@@               Coverage Diff                @@
##             master       #329        +/-   ##
================================================
- Coverage   59.9403%   59.9221%   -0.0183%     
================================================
  Files           135        135                
  Lines         15090      15158        +68     
================================================
+ Hits           9045       9083        +38     
- Misses         5166       5182        +16     
- Partials        879        893        +14

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

/run-all-tests

@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

1 similar comment
@sykp241095
Copy link
Member

/run-all-tests

dm/worker/config.go Outdated Show resolved Hide resolved
dm/worker/config.go Outdated Show resolved Hide resolved
dm/worker/config.go Outdated Show resolved Hide resolved
dm/worker/config.go Outdated Show resolved Hide resolved
dm/worker/config.go Outdated Show resolved Hide resolved
pkg/utils/db.go Outdated Show resolved Hide resolved
dm/worker/config.go Outdated Show resolved Hide resolved
@WangXiangUSTC
Copy link
Contributor Author

@csuzhangxc all addressed, PTAL again

pkg/utils/db.go Outdated Show resolved Hide resolved
syncer/online_ddl.go Outdated Show resolved Hide resolved
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.

rest LGTM

dm/worker/config.go Outdated Show resolved Hide resolved
c.Flavor, err = utils.GetFlavor(ctx, fromDB.DB)
if ctx.Err() != nil {
err = terror.Annotatef(err, "time cost to get flavor info exceeds %s", flavorGetTimeout)
rand.Seed(time.Now().UnixNano())
Copy link
Member

Choose a reason for hiding this comment

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

can we do this when starting the process?

Copy link
Contributor Author

@WangXiangUSTC WangXiangUSTC Oct 25, 2019

Choose a reason for hiding this comment

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

address comment b1a8fbd

syncer/heartbeat.go Show resolved Hide resolved
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.

rand.Seed(time.Now().UnixNano())
for i := 0; i < 5; i++ {
randomValue := uint32(rand.Intn(100000))
randomServerID := maxServerID/10 + randomValue
Copy link
Contributor

Choose a reason for hiding this comment

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

It sames that the only use of maxServerID is maxServerID/10. How about set maxServerID directly to math.MaxUint32/10?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update 354a2fc

Copy link
Contributor

@lichunzhu lichunzhu Oct 25, 2019

Choose a reason for hiding this comment

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

What about storing math.MaxUint32/10 into a variable so that we don't need to compute it every time?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 9b89300

@WangXiangUSTC
Copy link
Contributor Author

@csuzhangxc @lichunzhu PTAL again

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 priority/normal Minor change, requires approval from ≥1 primary reviewer and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Oct 25, 2019
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 Oct 25, 2019
@csuzhangxc
Copy link
Member

/run-all-tests

@csuzhangxc csuzhangxc merged commit eecbead into master Oct 25, 2019
@sre-bot
Copy link

sre-bot commented Oct 25, 2019

cherry pick to release-1.0 failed

@csuzhangxc
Copy link
Member

@WangXiangUSTC Please cherry-pick to release-1.0 manually.

@WangXiangUSTC WangXiangUSTC 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 already-update-docs The docs related to this PR already updated. Add this label once the docs are updated 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 needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated labels Oct 28, 2019
@csuzhangxc csuzhangxc added already-update-release-note The release note is updated. Add this label once the release note is updated and removed already-update-docs The docs related to this PR already updated. Add 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 Nov 1, 2019
@WangXiangUSTC WangXiangUSTC deleted the xiang/server_id branch January 9, 2020 06:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 status/LGT2 Two reviewers already commented LGTM, ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants