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

server/schedule: use ChangePeerV2 to demote single voter directly #4445

Closed
wants to merge 4 commits into from
Closed

server/schedule: use ChangePeerV2 to demote single voter directly #4445

wants to merge 4 commits into from

Conversation

w41ter
Copy link
Contributor

@w41ter w41ter commented Dec 9, 2021

What problem does this PR solve?

ref #4444.
ref tikv/tikv#11463.

What is changed and how it works?

Change the field ChangePeer to ChangePeerV2 if demoting a voter directlry. This can avoid breaking compatibility.

Check List

Tests

  • No code

Related changes

  • Need to cherry-pick to the release branch

Release note

Fix the problem that demoting a follower to learner isn't working.

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Dec 9, 2021
@ti-chi-bot ti-chi-bot requested review from disksing and Yisaer December 9, 2021 08:28
@w41ter
server/schedule: use ChangePeerV2 to demote single voter directly
c21656e 
ref tikv#4444.

Signed-off-by: p4tr1ck <patricknicholas@foxmail.com>
w41ter added a commit to w41ter/kvproto that referenced this pull request Dec 9, 2021
@w41ter
pdpb: update RegionHeartbeatResponse comments
df9783c 
This is part of work tikv/pd#4445.

Signed-off-by: p4tr1ck <patricknicholas@foxmail.com>
@w41ter w41ter mentioned this pull request Dec 9, 2021
Merged
@w41ter
Copy link
Contributor Author

w41ter commented Dec 9, 2021

In addition to this PR, please also help me to review : pingcap/kvproto#839.

w41ter added a commit to w41ter/kvproto that referenced this pull request Dec 13, 2021
@w41ter
pdpb: update RegionHeartbeatResponse comments
94a289f 
This is part of work tikv/pd#4445.

Signed-off-by: p4tr1ck patricknicholas@foxmail.com
w41ter added a commit to pingcap/kvproto that referenced this pull request Dec 13, 2021
@w41ter
pdpb: update RegionHeartbeatResponse comments (#839)
d0692ee 
This is part of work tikv/pd#4445.

Signed-off-by: p4tr1ck patricknicholas@foxmail.com
@w41ter
Merge remote-tracking branch 'origin/master' into fix_tikv_11463
e9f4179 
Signed-off-by: p4tr1ck <patricknicholas@foxmail.com>
@codecov
Copy link

codecov bot commented Dec 13, 2021 •
edited
Loading

Codecov Report

Merging #4445 (0b38d02) into master (3c626f2) will increase coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4445      +/-   ##
==========================================
+ Coverage   74.78%   74.82%   +0.03%     
==========================================
  Files         264      264              
  Lines       27789    27798       +9     
==========================================
+ Hits        20783    20799      +16     
- Misses       5140     5145       +5     
+ Partials     1866     1854      -12
Flag Coverage Δ
unittests 74.82% <0.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
server/schedule/operator/step.go 72.41% <0.00%> (-2.32%) ⬇️
server/schedule/operator_controller.go 83.50% <0.00%> (-1.23%) ⬇️
server/schedulers/random_merge.go 60.00% <0.00%> (-3.34%) ⬇️
server/member/member.go 63.10% <0.00%> (-2.14%) ⬇️
server/tso/allocator_manager.go 64.50% <0.00%> (-1.50%) ⬇️
server/grpc_service.go 51.78% <0.00%> (+0.19%) ⬆️
client/client.go 72.78% <0.00%> (+0.21%) ⬆️
server/config/persist_options.go 92.42% <0.00%> (+0.75%) ⬆️
server/server.go 72.35% <0.00%> (+0.88%) ⬆️
... and 6 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 3c626f2...0b38d02. Read the comment docs.

@w41ter
Copy link
Contributor Author

w41ter commented Dec 13, 2021

/test

@w41ter
Copy link
Contributor Author

w41ter commented Dec 14, 2021

/run-all-tests

@HunDunDM
Copy link
Member

Since ChangePeerV2 is used, useJointConsensus == true is required. This shows that the judgment condition of useJointConsensus and the logic of buildStepsWithJointConsensus need to be changed. At the same time, DemoteFollower will not be used in buildStepsWithoutJointConsensus, it must be split into RemovePeer and AddLearner.

@HunDunDM
Copy link
Member

We need a new way of implementation.

@HunDunDM
Copy link
Member

/hold

@ti-chi-bot ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2021
@disksing disksing removed their request for review December 21, 2021 05:24
@nolouch
Copy link
Contributor

nolouch commented Dec 27, 2021

ptal @HunDunDM

bufferflies
bufferflies reviewed Dec 28, 2021
View reviewed changes
server/schedule/operator/step.go
@@ -409,6 +409,20 @@ func (df DemoteFollower) CheckInProgress(cluster opt.Cluster, region *core.Regio
// Influence calculates the store difference that current step makes.
func (df DemoteFollower) Influence(opInfluence OpInfluence, region *core.RegionInfo) {}

// GetRequest get the ChangePeerV2 request
func (df DemoteFollower) GetRequest() *pdpb.ChangePeerV2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about add unit test?

@HunDunDM
Copy link
Member

HunDunDM commented Jan 5, 2022

#4444 is implemented in #4534, maybe we can close this PR? If you have any questions about #4534, please ask.

@w41ter w41ter closed this Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bufferflies bufferflies bufferflies left review comments

@Yisaer Yisaer Awaiting requested review from Yisaer

Assignees
No one assigned
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@w41ter @ti-chi-bot @HunDunDM @nolouch @bufferflies