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

*: [wip]Apply the configuration changes immediately in the followers #1467

Closed
wants to merge 13 commits into from

Conversation

bradyjoestar
Copy link
Contributor

@bradyjoestar bradyjoestar commented Mar 20, 2019

What problem does this PR solve?

#1466 issue-1466

What is changed and how it works?

  • if config changed by confHandler, leader and follower apply it instantly

wait for watch service.

@sre-bot
Copy link
Contributor

sre-bot commented Mar 20, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

1 similar comment
@sre-bot
Copy link
Contributor

sre-bot commented Mar 20, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@CLAassistant
Copy link

CLAassistant commented Mar 20, 2019

CLA assistant check
All committers have signed the CLA.

@zhouqiang-cl
Copy link
Contributor

/rebuild

server/leader.go Outdated
@@ -292,6 +292,21 @@ func (s *Server) campaignLeader() error {
log.Info("server is closed")
return nil
}

// leader apply config
configHash, err := s.kv.LoadConfigHash()
Copy link
Contributor

Choose a reason for hiding this comment

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

Leader no need to reload, it will be set firstly then persist to KV.

server/leader.go Outdated
@@ -318,6 +333,21 @@ func (s *Server) watchLeader(leader *pdpb.Member, revision int64) {
// If the revision is compacted, will meet required revision has been compacted error.
// In this case, use the compact revision to re-watch the key.
for {
//follower watch config change and apply it
configHash, err := s.kv.LoadConfigHash()
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic will not be executed periodically or immediately. the rch will block until the leader changed or some error meet.

@nolouch nolouch changed the title Issue 1466 *: Modify the configuration changes immediately in the followers Mar 20, 2019
@nolouch nolouch changed the title *: Modify the configuration changes immediately in the followers *: Apply the configuration changes immediately in the followers Mar 20, 2019
@nolouch
Copy link
Contributor

nolouch commented Mar 20, 2019

@bradyjoestar Plase signs the cla.

@bradyjoestar
Copy link
Contributor Author

@nolouch I will do it tonight and try to find why ci isn't passed. 🍺

@bradyjoestar
Copy link
Contributor Author

/rebuild

@rleungx
Copy link
Member

rleungx commented Mar 20, 2019

@bradyjoestar For the CI problem, I have raised a PR to make it more stable. See #1471.

@bradyjoestar
Copy link
Contributor Author

@rleungx got it.

Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

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

We need a test to verify it.

break
}
}
time.Sleep(200 * time.Millisecond)
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 it is too frequent to read from KV, maybe we can use Watch API like the leader key to listen key changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @bradyjoestar, after we discussed, there is a better solution. Because we not only PD need to dynamically load the configuration, but also TiKV needs. we can add a service in gRPC that provides the Watch function. If you are interested, you can send your wechat to my email. chenshuning@pingcap.com

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nolunch Have send to your email. It sounds cool 😸

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@f72cc23). Click here to learn what that means.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1467   +/-   ##
=========================================
  Coverage          ?   67.43%           
=========================================
  Files             ?      158           
  Lines             ?    15364           
  Branches          ?        0           
=========================================
  Hits              ?    10361           
  Misses            ?     4062           
  Partials          ?      941
Impacted Files Coverage Δ
server/core/kv.go 79.16% <100%> (ø)
server/leader.go 85.25% <100%> (ø)
server/option.go 89.65% <71.42%> (ø)

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 f72cc23...e09d17b. Read the comment docs.

@bradyjoestar bradyjoestar changed the title *: Apply the configuration changes immediately in the followers *: [wip]Apply the configuration changes immediately in the followers Mar 28, 2019
@bradyjoestar
Copy link
Contributor Author

I am recently focuing on libos in the intel-sgx, so sorry for delaying this pr.
I'd like to close this pr temporary.

Sorry!
webb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants