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.

Sign up for GitHub

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

Jump to bottom

config: introduce a config client to support load configs from PD online #14303

Merged
merged 19 commits into from
Feb 10, 2020
Merged

config: introduce a config client to support load configs from PD online #14303

merged 19 commits into from
Feb 10, 2020

Conversation

qw4990
Copy link
Contributor

@qw4990 qw4990 commented Dec 31, 2019 •
edited
Loading

What problem does this PR solve?

Part I to implement this proposal: #13660.
Wait for tikv/pd#2041.
In this PR, we introduce a config client(ConfHandler) to support load configs from PD online.

What is changed and how it works?

  1. Move the method parsePath from kv package to config package to avoid recycle import.
  2. Add a new method MarshalText to nullableBool to make it can be encoded and decoded correctly.
  3. Introduce ConfHandler to support to load configs from PD online.

Check List

Tests

  • Unit test

@qw4990
Copy link
Contributor Author

qw4990 commented Jan 7, 2020

/rebuild

@qw4990
Copy link
Contributor Author

qw4990 commented Jan 7, 2020

/rebuild

1 similar comment
@you06
Copy link
Contributor

you06 commented Jan 7, 2020

/rebuild

@qw4990 qw4990 mentioned this pull request Jan 8, 2020
Closed
@qw4990
Copy link
Contributor Author

qw4990 commented Jan 8, 2020

/run-unit-test

1 similar comment
@qw4990
Copy link
Contributor Author

qw4990 commented Jan 8, 2020

/run-unit-test

@SunRunAway SunRunAway mentioned this pull request Jan 20, 2020
Closed
55 tasks
@qw4990 qw4990 requested a review from wshwsh12 February 5, 2020 06:30
wshwsh12
wshwsh12 reviewed Feb 5, 2020
View reviewed changes
Copy link
Contributor

@wshwsh12 wshwsh12 left a comment

Choose a reason for hiding this comment

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

LGTM

config/config_handler.go Outdated Show resolved Hide resolved
config/config_handler_test.go Outdated Show resolved Hide resolved
rleungx
rleungx reviewed Feb 6, 2020
View reviewed changes
config/config_handler.go Outdated Show resolved Hide resolved
config/config_handler.go Outdated Show resolved Hide resolved
XuHuaiyu
XuHuaiyu reviewed Feb 6, 2020
View reviewed changes
config/config_handler.go Show resolved Hide resolved
config/config_handler.go Outdated Show resolved Hide resolved
config/config_handler.go Show resolved Hide resolved
config/config_handler.go Outdated Show resolved Hide resolved
config/config_handler.go Outdated Show resolved Hide resolved
@qw4990 qw4990 added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 7, 2020
@qw4990
Copy link
Contributor Author

qw4990 commented Feb 7, 2020

@XuHuaiyu @rleungx All comments have been addressed, PTAL~

@qw4990
Copy link
Contributor Author

qw4990 commented Feb 7, 2020

/rebuild

XuHuaiyu
XuHuaiyu reviewed Feb 10, 2020
View reviewed changes
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

Addressing the comment makes a LGTM

config/config.go Outdated Show resolved Hide resolved
@qw4990
Copy link
Contributor Author

qw4990 commented Feb 10, 2020

/run-all-tests

@rleungx
Copy link
Member

rleungx commented Feb 10, 2020

CI failed.

@qw4990
Copy link
Contributor Author

qw4990 commented Feb 10, 2020

/run-all-tests

XuHuaiyu
XuHuaiyu reviewed Feb 10, 2020
View reviewed changes
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@qw4990
Copy link
Contributor Author

qw4990 commented Feb 10, 2020

/run-unit-test

@qw4990 qw4990 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 10, 2020
@qw4990 qw4990 removed the request for review from SunRunAway February 10, 2020 06:45
@qw4990
Copy link
Contributor Author

qw4990 commented Feb 10, 2020

@rleungx All comments have been addressed, PTAL. And please help me to approve this PR if there is no problem.

rleungx
rleungx approved these changes Feb 10, 2020
View reviewed changes
Copy link
Member

@rleungx rleungx left a comment

Choose a reason for hiding this comment

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

LGTM

@qw4990 qw4990 merged commit ad1d07f into pingcap:master Feb 10, 2020
@qw4990 qw4990 mentioned this pull request Feb 12, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XuHuaiyu XuHuaiyu XuHuaiyu left review comments

@wshwsh12 wshwsh12 wshwsh12 left review comments

@rleungx rleungx rleungx approved these changes

@NingLin-P NingLin-P Awaiting requested review from NingLin-P

Assignees
No one assigned
Labels
component/config sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@qw4990 @you06 @rleungx @XuHuaiyu @wshwsh12