-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Split apart and test appendConfigurationEntry #132
Split apart and test appendConfigurationEntry #132
Conversation
This fixes a bug where AddNonvoter would previously demote a server from Staging to Nonvoter (the intended semantics are no-op in that case). This is hashicorp#14 on James's post-PR-117 checklist.
// it'll instruct the replication routines to try to replicate to the current | ||
// index. | ||
func (r *Raft) startStopReplication() { | ||
inConfig := make(map[ServerID]bool, len(r.leaderState.replState)) |
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.
This should probably be len(r.configurations.latest.Servers)
. This'll be 0 at this time.
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.
Nevermind - not for the stop case.
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.
Even then, you're still right: there'll be one entry in the map per len(r.configurations.latest.Servers), perhaps except for the local server.
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.
Yeah it can't hurt to use that as the size for sure.
@ongardie just a few minor comments, otherwise LGTM! |
This simplifies the signature for nextConfiguration. In response to comments on PR#132.
Pushed a few more, I think that addressed all the comments. |
Looks good - thanks! |
This fixes a bug where AddNonvoter would previously demote a server from
Staging to Nonvoter (the intended semantics are no-op in that case).
This is box 14 on James's post- #117 checklist on #84.