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

*: add config gRPC service #2033

Merged
merged 7 commits into from
Dec 30, 2019
Merged

*: add config gRPC service #2033

merged 7 commits into from
Dec 30, 2019

Conversation

rleungx
Copy link
Member

@rleungx rleungx commented Dec 18, 2019

What problem does this PR solve?

Part of #1977. It should be reviewed after #2018 is merged.

What is changed and how it works?

This PR adds the config gRPC service. Also it adds EnableConfigManager in command line to decide whether we enable configuration manager.

Check List

Tests

  • Unit test

@rleungx rleungx added the component/config Configuration logic. label Dec 18, 2019
@rleungx rleungx added this to the v4.0.0-beta milestone Dec 18, 2019
@codecov-io
Copy link

codecov-io commented Dec 18, 2019 •
edited
Loading

Codecov Report

Merging #2033 into master will decrease coverage by 0.2%.
The diff coverage is 10.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2033      +/-   ##
==========================================
- Coverage   76.79%   76.58%   -0.21%     
==========================================
  Files         186      187       +1     
  Lines       19049    19092      +43     
==========================================
- Hits        14628    14621       -7     
- Misses       3305     3350      +45     
- Partials     1116     1121       +5
Impacted Files Coverage Δ
server/config/config.go 84.61% <ø> (-1.1%) ⬇️
server/config_manager/grpc_service.go 0% <0%> (ø)
server/config_manager/config_manager.go 73.75% <100%> (+0.09%) ⬆️
server/server.go 81.85% <16.66%> (-0.75%) ⬇️
server/member/leader.go 76.53% <0%> (-4.6%) ⬇️
pkg/etcdutil/etcdutil.go 88.4% <0%> (-2.9%) ⬇️
server/schedule/operator_controller.go 83.78% <0%> (+0.18%) ⬆️
pkg/mock/mockhbstream/mockhbstream.go 90.76% <0%> (+1.53%) ⬆️
... and 2 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 a607642...f9969e2. Read the comment docs.

disksing
disksing reviewed Dec 18, 2019
View reviewed changes
server/config/config.go
@@ -129,6 +129,8 @@ type Config struct {

logger *zap.Logger
logProps *log.ZapProperties

EnableConfigManager bool
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can enable it by default. No need the config. If no client use it, the feature has no side effect.

Copy link
Member Author

Choose a reason for hiding this comment

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

The switch will also control the PD config management which will start a goroutine in #2049.

@rleungx rleungx mentioned this pull request Dec 19, 2019
Merged
shafreeck
shafreeck reviewed Dec 26, 2019
View reviewed changes
server/config_manager/config_manager.go
@@ -43,18 +45,29 @@ var (
errNotSupported = "not supported"
)

// Server is the interface for configuration manager.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it seem that we have another interface called Server? Maybe we should define the interface with some specifications.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and actually we indeed have some interfaces called Server in different packages. Maybe we can handle it in the future. But I don't have a good idea.

overvenus
overvenus reviewed Dec 27, 2019
View reviewed changes
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.

Rest LGTM

server/server.go Show resolved Hide resolved
lhy1024
lhy1024 reviewed Dec 27, 2019
View reviewed changes
Copy link
Contributor

@lhy1024 lhy1024 left a comment

Choose a reason for hiding this comment

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

LGTM

@NingLin-P
Copy link
Member

LGTM

@rleungx
add config manager
d4da319 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
address comments
f993471 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
add config gRPC service
fd2460d 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
remove bootstrap check
1e24cde 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
address the comment
759005e 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
Copy link
Member Author

rleungx commented Dec 30, 2019

/run-all-tests

overvenus
overvenus approved these changes Dec 30, 2019
View reviewed changes
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

@rleungx rleungx added the status/can-merge Indicates a PR has been approved by a committer. label Dec 30, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Dec 30, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Dec 30, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Dec 30, 2019

@rleungx merge failed.

@rleungx
Copy link
Member Author

rleungx commented Dec 30, 2019

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Dec 30, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Dec 30, 2019

@rleungx merge failed.

@rleungx
Copy link
Member Author

rleungx commented Dec 30, 2019

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Dec 30, 2019

/run-all-tests

@sre-bot sre-bot merged commit cdb5973 into tikv:master Dec 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhy1024 lhy1024 lhy1024 left review comments

@disksing disksing disksing left review comments

@shafreeck shafreeck shafreeck left review comments

@NingLin-P NingLin-P NingLin-P approved these changes

@overvenus overvenus overvenus approved these changes

Assignees
No one assigned
Labels
component/config Configuration logic. status/can-merge Indicates a PR has been approved by a committer.
Projects
None yet
Milestone
v4.0.0-beta
Development

Successfully merging this pull request may close these issues.
8 participants
@rleungx @codecov-io @NingLin-P @sre-bot @shafreeck @overvenus @disksing @lhy1024