-
Notifications
You must be signed in to change notification settings - Fork 727
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
server, core: support add learner node #896
Conversation
server/config.go
Outdated
@@ -333,6 +333,8 @@ type ScheduleConfig struct { | |||
ReplicaScheduleLimit uint64 `toml:"replica-schedule-limit,omitempty" json:"replica-schedule-limit"` | |||
// TolerantSizeRatio is the ratio of buffer size for balance scheduler. | |||
TolerantSizeRatio float64 `toml:"tolerant-size-ratio,omitempty" json:"tolerant-size-ratio"` | |||
// EnableRaftLearner is the switch for using AddLearnerNode instead of AddNode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch -> option.
server/coordinator.go
Outdated
@@ -472,6 +472,32 @@ func (c *coordinator) sendScheduleCommand(region *core.RegionInfo, step schedule | |||
}, | |||
} | |||
c.hbStreams.sendMsg(region, cmd) | |||
case schedule.AddLearnerPeer: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove a learner peer? I believe we need this feature in some cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can reuse RemovePeer
server/core/region.go
Outdated
DownPeers []*pdpb.PeerStats | ||
PendingPeers []*metapb.Peer | ||
PendingLearnerPeers []*metapb.Peer | ||
CompleteLearnerPeers []*metapb.Peer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any document about how the CompleteLearner
works? Specifically, when will a peer be added to the list and when will it be removed?
server/schedule/operator.go
Outdated
|
||
// Influence calculates the store difference that current step make | ||
func (plp PromoteLearnerPeer) Influence(opInfluence OpInfluence, region *core.RegionInfo) { | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just leave the method body empty.
server/schedule/operator.go
Outdated
// IsFinish checks if current step is finished. | ||
func (alp AddLearnerPeer) IsFinish(region *core.RegionInfo) bool { | ||
if p := region.GetStorePeer(alp.ToStore); p != nil { | ||
return region.GetPendingLearnerPeer(p.GetId()) == nil && region.GetCompleteLearnerPeer(p.GetId()) != nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIU, a learner peer will be included in the peers
list. This fact will affect some old utilities, such as RandomFollower
. We should not select a learner as follower in some cases, for example we cannot transfer leader to a learner peer.
We should consider how to recover from unfinished operator. If pd's leader is changed, we need to remove a learner or promote it. |
998e14b
to
48c5e88
Compare
server/core/region.go
Outdated
} | ||
|
||
// ClassifyVoterAndLearner sorts out voter and learner from peers into different slice. | ||
func ClassifyVoterAndLearner(region *RegionInfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to export this function?
server/config.go
Outdated
@@ -392,6 +395,7 @@ const ( | |||
defaultReplicaScheduleLimit = 32 | |||
defaultMergeScheduleLimit = 20 | |||
defaultTolerantSizeRatio = 2.5 | |||
defaultEnableRaftLearner = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remote this unused variable.
server/core/region.go
Outdated
@@ -119,6 +157,26 @@ func (r *RegionInfo) GetDownPeer(peerID uint64) *metapb.Peer { | |||
return nil | |||
} | |||
|
|||
// GetDownVoter returns the down peer with specified peer id. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returns the down voter
https://github.com/pingcap/pd/blob/e221ffb59f7b3433ee2e0a617f616dc92b02d007/server/schedule/replica_checker.go#L188 |
https://github.com/pingcap/pd/blob/e221ffb59f7b3433ee2e0a617f616dc92b02d007/server/schedule/merge_checker.go#L40-L43 |
LGTM. |
server/core/region.go
Outdated
followers map[uint64]*regionMap // storeID -> regionID -> regionInfo | ||
learners map[uint64]*regionMap // storeID -> regionID -> regionInfo | ||
pendingPeers map[uint64]*regionMap // storeID -> regionID -> regionInfo | ||
pendingLearners map[uint64]*regionMap // storeID -> regionID -> regionInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove pendingLearners
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
8173c88
to
6adc7e7
Compare
server/schedule/operator.go
Outdated
@@ -79,7 +79,7 @@ func (ap AddPeer) IsFinish(region *core.RegionInfo) bool { | |||
log.Warnf("expect %v, but obtain voter %v", ap.String(), p.GetId()) | |||
return false | |||
} | |||
return region.GetPendingVoter(p.GetId()) == nil && p.GetId() != ap.PeerID | |||
return region.GetPendingVoter(p.GetId()) == nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because AddNode
won't have any subsequence operators?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think above if
block already checked if their IDs are matched, so we don't need it here.
99737d8
to
ef7ebe8
Compare
/run-all-tests |
LGTM. |
this pr does:
note that: peers = voters+learners