checker: avoid unnecessary remove disconnected peer with multi orphan peers #7315

lhy1024 · 2023-11-03T03:43:28Z

What problem does this PR solve?

Issue Number: Close #7249

When there are many orphan peers, we don't think disconnected peer are healthy
And when we decide to remove peer, we will pick disconnected peer firstly.

What is changed and how does it work?

Check List

Tests

Unit test
Integration test

Release note

None.

ti-chi-bot · 2023-11-03T03:43:31Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

nolouch
rleungx

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.

Signed-off-by: lhy1024 <admin@liudos.us>

codecov · 2023-11-03T11:45:15Z

Codecov Report

Merging #7315 (f12d0b4) into master (ab8bf7b) will decrease coverage by 0.03%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master    #7315      +/-   ##
==========================================
- Coverage   74.49%   74.46%   -0.03%     
==========================================
  Files         446      446              
  Lines       48346    48352       +6     
==========================================
- Hits        36016    36007       -9     
- Misses       9160     9165       +5     
- Partials     3170     3180      +10

Flag	Coverage Δ
unittests	`74.46% <84.61%> (-0.03%)`	⬇️

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

lhy1024 · 2023-11-03T11:46:01Z

pkg/schedule/checker/rule_checker.go

@@ -534,8 +534,7 @@ func (c *RuleChecker) fixOrphanPeers(region *core.RegionInfo, fit *placement.Reg
 					return operator.CreatePromoteLearnerOperatorAndRemovePeer("replace-down-peer-with-orphan-peer", c.cluster, region, orphanPeer, pinDownPeer)
 				case orphanPeerRole == metapb.PeerRole_Voter && destRole == metapb.PeerRole_Learner:
 					return operator.CreateDemoteLearnerOperatorAndRemovePeer("replace-down-peer-with-orphan-peer", c.cluster, region, orphanPeer, pinDownPeer)
-				case orphanPeerRole == metapb.PeerRole_Voter && destRole == metapb.PeerRole_Voter &&
-					isDisconnectedPeer(pinDownPeer) && !dstStore.IsDisconnected():
+				case orphanPeerRole == destRole && isDisconnectedPeer(pinDownPeer) && !dstStore.IsDisconnected():


allow replace learner

lhy1024 · 2023-11-03T11:46:23Z

pkg/schedule/checker/rule_checker.go

 			}
 			if hasHealthPeer {
 				// there already exists a healthy orphan peer, so we can remove other orphan Peers.
 				ruleCheckerRemoveOrphanPeerCounter.Inc()
+				// if there exists a disconnected orphan peer, we will pick it to remove firstly.


avoid to remove normal peer

Consider that we have 3 orphan peer, two healthy and one disconnected. Is it possiable that we remove a healthy peer first?

Should we always remove disconnected peer first?

Should we always remove disconnected peer first?

I think so.

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 · 2023-11-06T05:31:11Z

@rleungx PTAL

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 · 2023-11-06T12:09:48Z

succesfully run test

no failure in tidb

all orphan peers are removed with 11min down store time config

lhy1024 · 2023-11-06T12:19:55Z

/merge

ti-chi-bot · 2023-11-06T12:19:58Z

@lhy1024: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-11-06T12:20:00Z

This pull request has been accepted and is ready to merge.

Commit hash: f12d0b4

… peers (tikv#7315) close tikv#7249 Signed-off-by: lhy1024 <admin@liudos.us>

ti-chi-bot bot added the release-note-none Denotes a PR that doesn't merit a release note. label Nov 3, 2023

ti-chi-bot bot requested review from nolouch and rleungx November 3, 2023 03:43

ti-chi-bot bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 3, 2023

nolouch approved these changes Nov 3, 2023

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 3, 2023

avoid directly remove disconnected peer

b4cbaba

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 force-pushed the fix-check3 branch from cb22816 to b4cbaba Compare November 3, 2023 11:45

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 3, 2023

lhy1024 commented Nov 3, 2023

View reviewed changes

lhy1024 requested a review from nolouch November 3, 2023 11:46

fix lint

d6e3a92

Signed-off-by: lhy1024 <admin@liudos.us>

nolouch approved these changes Nov 6, 2023

View reviewed changes

refactor

1235d1c

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 added 3 commits November 6, 2023 13:35

Merge branch 'master' into fix-check3

c3c16ea

refactor

bb7a892

Signed-off-by: lhy1024 <admin@liudos.us>

address comments and add more tests

b290891

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx approved these changes Nov 6, 2023

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 6, 2023

lhy1024 added 3 commits November 6, 2023 15:11

add more test

88d6ecc

Signed-off-by: lhy1024 <admin@liudos.us>

add more test

b60ffca

Signed-off-by: lhy1024 <admin@liudos.us>

add metrics

f12d0b4

Signed-off-by: lhy1024 <admin@liudos.us>

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 6, 2023

ti-chi-bot bot merged commit c332ddc into tikv:master Nov 6, 2023
26 checks passed

lhy1024 added a commit to lhy1024/pd that referenced this pull request Nov 6, 2023

checker: avoid unnecessary remove disconnected peer with multi orphan…

04cb434

… peers (tikv#7315) close tikv#7249 Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 added a commit to lhy1024/pd that referenced this pull request Nov 8, 2023

checker: avoid unnecessary remove disconnected peer with multi orphan…

6e4981f

… peers (tikv#7315) close tikv#7249 Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 mentioned this pull request Nov 8, 2023

checker: reduces the probability of deleting normal peers when the store becomes unavailable (#7249) #7330

Merged

lhy1024 added a commit to lhy1024/pd that referenced this pull request Nov 8, 2023

checker: avoid unnecessary remove disconnected peer with multi orphan…

ddee72c

… peers (tikv#7315) close tikv#7249 Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 added a commit to lhy1024/pd that referenced this pull request Nov 8, 2023

checker: avoid unnecessary remove disconnected peer with multi orphan…

6a590d0

… peers (tikv#7315) close tikv#7249 Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 mentioned this pull request Nov 8, 2023

checker: reduces the probability of deleting normal peers when the store becomes unavailable (#7249) #7334

Merged

lhy1024 added a commit to lhy1024/pd that referenced this pull request Nov 8, 2023

checker: avoid unnecessary remove disconnected peer with multi orphan…

545c23d

… peers (tikv#7315) close tikv#7249 Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 mentioned this pull request Nov 8, 2023

checker: reduces the probability of deleting normal peers when the store becomes unavailable (#7249) #7337

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

checker: avoid unnecessary remove disconnected peer with multi orphan peers #7315

checker: avoid unnecessary remove disconnected peer with multi orphan peers #7315

lhy1024 commented Nov 3, 2023 •

edited

Loading

ti-chi-bot bot commented Nov 3, 2023 •

edited

Loading

codecov bot commented Nov 3, 2023 •

edited

Loading

lhy1024 Nov 3, 2023

lhy1024 Nov 3, 2023

rleungx Nov 6, 2023

rleungx Nov 6, 2023

lhy1024 Nov 6, 2023 •

edited

Loading

lhy1024 commented Nov 6, 2023

lhy1024 commented Nov 6, 2023

lhy1024 commented Nov 6, 2023

ti-chi-bot bot commented Nov 6, 2023

ti-chi-bot bot commented Nov 6, 2023

checker: avoid unnecessary remove disconnected peer with multi orphan peers #7315

checker: avoid unnecessary remove disconnected peer with multi orphan peers #7315

Conversation

lhy1024 commented Nov 3, 2023 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Nov 3, 2023 • edited Loading

codecov bot commented Nov 3, 2023 • edited Loading

Codecov Report

lhy1024 Nov 3, 2023

Choose a reason for hiding this comment

lhy1024 Nov 3, 2023

Choose a reason for hiding this comment

rleungx Nov 6, 2023

Choose a reason for hiding this comment

rleungx Nov 6, 2023

Choose a reason for hiding this comment

lhy1024 Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

lhy1024 commented Nov 6, 2023

lhy1024 commented Nov 6, 2023

lhy1024 commented Nov 6, 2023

ti-chi-bot bot commented Nov 6, 2023

ti-chi-bot bot commented Nov 6, 2023

lhy1024 commented Nov 3, 2023 •

edited

Loading

ti-chi-bot bot commented Nov 3, 2023 •

edited

Loading

codecov bot commented Nov 3, 2023 •

edited

Loading

lhy1024 Nov 6, 2023 •

edited

Loading