Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: fix schedulers maybe lost after restarting #2154

Merged
merged 2 commits into from
Feb 27, 2020

Conversation

rleungx
Copy link
Member

@rleungx rleungx commented Feb 25, 2020

What problem does this PR solve?

When we enable dynamic config, if we use pd-ctl to add a scheduler, it won't update the config manager and it will be lost after restarting. It should be reviewed after #2153 is merged.

What is changed and how it works?

This PR adds a callback function to update the config manager to solve the above problem.

Check List

Tests

  • Manual test
  1. start PD
  2. add a scheduler via pd-ctl
  3. restart PD
  4. use pd-ctl config show to check

@rleungx rleungx added type/bug The issue is confirmed as a bug. component/config Configuration logic. labels Feb 25, 2020
@rleungx rleungx added this to the v4.0.0-rc milestone Feb 25, 2020
@codecov-io
Copy link

Codecov Report

Merging #2154 into master will increase coverage by 0.22%.
The diff coverage is 67.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2154      +/-   ##
==========================================
+ Coverage   76.01%   76.24%   +0.22%     
==========================================
  Files         195      195              
  Lines       20603    20626      +23     
==========================================
+ Hits        15662    15726      +64     
+ Misses       3743     3713      -30     
+ Partials     1198     1187      -11
Impacted Files Coverage Δ
server/cluster/cluster.go 81.62% <100%> (+0.47%) ⬆️
server/cluster/coordinator.go 78.39% <100%> (+0.12%) ⬆️
server/grpc_service.go 58.15% <33.33%> (+1.07%) ⬆️
server/server.go 79.12% <62.5%> (+0.63%) ⬆️
server/config_manager/config_manager.go 79.05% <71.42%> (+2.41%) ⬆️
server/schedulers/random_merge.go 60.71% <0%> (-3.58%) ⬇️
pkg/mock/mockhbstream/mockhbstream.go 89.23% <0%> (-1.54%) ⬇️
server/config/option.go 90.04% <0%> (+0.86%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05d6873...02171fb. Read the comment docs.

Copy link
Contributor

@shafreeck shafreeck left a comment

Choose a reason for hiding this comment

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

LGTM

@disksing disksing removed their request for review February 27, 2020 04:18
Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

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

LGTM, better to add a test though

@rleungx
Copy link
Member Author

rleungx commented Feb 27, 2020

/run-all-tests

@rleungx
Copy link
Member Author

rleungx commented Feb 27, 2020

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 27, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Feb 27, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Feb 27, 2020

@rleungx merge failed.

@rleungx
Copy link
Member Author

rleungx commented Feb 27, 2020

/run-all-tests

@rleungx
Copy link
Member Author

rleungx commented Feb 27, 2020

/build

@claassistantio
Copy link

claassistantio commented Feb 27, 2020

CLA assistant check
All committers have signed the CLA.

@rleungx
Copy link
Member Author

rleungx commented Feb 27, 2020

/run-all-tests

@rleungx
Copy link
Member Author

rleungx commented Feb 27, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Feb 27, 2020

/run-all-tests

@sre-bot sre-bot merged commit 0247a5d into tikv:master Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/config Configuration logic. status/can-merge Indicates a PR has been approved by a committer. type/bug The issue is confirmed as a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants