Validate leader value in election #153
Conversation
aggregator/election_mgr.go
Outdated
| @@ -329,6 +334,7 @@ func (mgr *electionManager) Open(shardSetID uint32) error { | |||
| if mgr.state != electionManagerNotOpen { | |||
| return errElectionManagerAlreadyOpenOrClosed | |||
| } | |||
| mgr.shardSetID = shardSetID | |||
There was a problem hiding this comment.
Is this just for testing. I dont see where this is used.
aggregator/election_mgr.go
Outdated
| mgr.logError("error getting placement", err) | ||
| return err | ||
| } | ||
| _, exist := p.Instance(leader) |
There was a problem hiding this comment.
Seems like this will just tell you if the host exists, but not if the electionKey for the which this host is the leader?
This would tell us if the leader is a valid host, but not if that host thinks it is the leader. Perhaps this might be helpful:
instance, exist := p.Instance(leader)
if !exists {
// handle unknown host as leader
}
if instance.ShardSetID() != mgr.ShardSetID {
// handle leader with different shardSetID
}
There was a problem hiding this comment.
Cool, will add the extra check
aggregator/election_mgr.go
Outdated
| if !exist { | ||
| mgr.metrics.verifyLeaderNotInPlacement.Inc(1) | ||
| err := fmt.Errorf("received invalid leader value: [%s], which is not available in placement", leader) | ||
| mgr.logger.Error(err.Error()) |
There was a problem hiding this comment.
Use mgr.logError here to be consistent?
aggregator/election_mgr.go
Outdated
| mgr.logger.Error(err.Error()) | ||
| return err | ||
| } | ||
| mgr.logger.Infof("found valid new leader: [%s] for the campaign", leader) |
There was a problem hiding this comment.
Do you need check if the current instance and the leader instance are in the same group?
aggregator/election_mgr.go
Outdated
| if leaderInstance.ShardSetID() != instance.ShardSetID() { | ||
| err := fmt.Errorf("received invalid leader value: [%s] which owns shardSet %v, while this aggregator owns shardSet %v", | ||
| leader, leaderInstance.ShardSetID(), instance.ShardSetID()) | ||
| mgr.logError("invalid leader value", err) |
There was a problem hiding this comment.
nit: "leader is in a different shardset group" might be a better error message.
This pr validates the value of the leader to make sure it's a valid instance id in the placement
@xichen2020 @robskillington @jeromefroe