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

Deprecate Staging suffrage and Promote command #486

Merged
merged 2 commits into from
Jan 24, 2022
Merged

Deprecate Staging suffrage and Promote command #486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08712c3
Update godoc for Staging and Promote
dnephin Jan 17, 2022
0a8bb8c
Deprecate AddStaging and Promote
dnephin Jan 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,7 +825,7 @@ func (r *Raft) AddPeer(peer ServerAddress) Future {
}

return r.requestConfigChange(configurationChangeRequest{
command: AddStaging,
command: AddVoter,
serverID: ServerID(peer),
serverAddress: peer,
prevIndex: 0,
Expand Down Expand Up @@ -862,7 +862,7 @@ func (r *Raft) AddVoter(id ServerID, address ServerAddress, prevIndex uint64, ti
}

return r.requestConfigChange(configurationChangeRequest{
command: AddStaging,
command: AddVoter,
serverID: id,
serverAddress: address,
prevIndex: prevIndex,
Expand Down
35 changes: 16 additions & 19 deletions configuration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ const (
// Nonvoter is a server that receives log entries but is not considered for
// elections or commitment purposes.
Nonvoter
// Staging is a server that acts like a nonvoter with one exception: once a
// staging server receives enough log entries to be sufficiently caught up to
// the leader's log, the leader will invoke a membership change to change
// the Staging server to a Voter.
// Staging is a server that acts like a Nonvoter. A configuration change
// with a ConfigurationChangeCommand of Promote can change a Staging server
// into a Voter.
// Deprecated: use Nonvoter instead.
Staging
)

Expand Down Expand Up @@ -87,23 +87,27 @@ func (c *Configuration) Clone() (copy Configuration) {
type ConfigurationChangeCommand uint8

const (
// AddStaging makes a server Staging unless its Voter.
AddStaging ConfigurationChangeCommand = iota
// AddVoter adds a server with Suffrage of Voter.
AddVoter ConfigurationChangeCommand = iota
// AddNonvoter makes a server Nonvoter unless its Staging or Voter.
AddNonvoter
// DemoteVoter makes a server Nonvoter unless its absent.
DemoteVoter
// RemoveServer removes a server entirely from the cluster membership.
RemoveServer
// Promote is created automatically by a leader; it turns a Staging server
// into a Voter.
// Promote changes a server from Staging to Voter. The command will be a
// no-op if the server is not Staging.
// Deprecated: use AddVoter instead.
Promote
// AddStaging makes a server a Voter.
// Deprecated: AddStaging was actually AddVoter. Use AddVoter instead.
AddStaging = 0 // explicit 0 to preserve the old value.
Comment on lines +102 to +104
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By keeping the constant value 0 it should be safe to upgrade raft. Because any existing clients will send a 0 in the RPC message, and new clients will also send a 0 using the new AddVoter constant.

)

func (c ConfigurationChangeCommand) String() string {
switch c {
case AddStaging:
return "AddStaging"
case AddVoter:
return "AddVoter"
case AddNonvoter:
return "AddNonvoter"
case DemoteVoter:
Expand All @@ -122,7 +126,7 @@ func (c ConfigurationChangeCommand) String() string {
type configurationChangeRequest struct {
command ConfigurationChangeCommand
serverID ServerID
serverAddress ServerAddress // only present for AddStaging, AddNonvoter
serverAddress ServerAddress // only present for AddVoter, AddNonvoter
// prevIndex, if nonzero, is the index of the only configuration upon which
// this change may be applied; if another configuration entry has been
// added in the meantime, this request will fail.
Expand Down Expand Up @@ -214,15 +218,8 @@ func nextConfiguration(current Configuration, currentIndex uint64, change config

configuration := current.Clone()
switch change.command {
case AddStaging:
// TODO: barf on new address?
case AddVoter:
newServer := Server{
// TODO: This should add the server as Staging, to be automatically
// promoted to Voter later. However, the promotion to Voter is not yet
// implemented, and doing so is not trivial with the way the leader loop
// coordinates with the replication goroutines today. So, for now, the
// server will have a vote right away, and the Promote case below is
// unused.
Suffrage: Voter,
ID: change.serverID,
Address: change.serverAddress,
Expand Down
16 changes: 13 additions & 3 deletions configuration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,16 @@ var nextConfigurationTests = []struct {
// AddStaging: was Nonvoter.
{oneOfEach, AddStaging, 3, "{[{Voter id1 addr1x} {Staging id2 addr2x} {Voter id3 addr3}]}"},

// AddVoter: was missing.
{Configuration{}, AddVoter, 1, "{[{Voter id1 addr1}]}"},
{singleServer, AddVoter, 2, "{[{Voter id1 addr1x} {Voter id2 addr2}]}"},
// AddVoter: was Voter.
{singleServer, AddVoter, 1, "{[{Voter id1 addr1}]}"},
// AddVoter: was Staging.
{oneOfEach, AddVoter, 2, "{[{Voter id1 addr1x} {Voter id2 addr2} {Nonvoter id3 addr3x}]}"},
// AddVoter: was Nonvoter.
{oneOfEach, AddVoter, 3, "{[{Voter id1 addr1x} {Staging id2 addr2x} {Voter id3 addr3}]}"},

Comment on lines +182 to +191
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just copy/paste of the AddStaging test cases above, with the second field change. I left the original test cases to show backwards compat.

// AddNonvoter: was missing.
{singleServer, AddNonvoter, 2, "{[{Voter id1 addr1x} {Nonvoter id2 addr2}]}"},
// AddNonvoter: was Voter.
Expand Down Expand Up @@ -238,7 +248,7 @@ func TestConfiguration_nextConfiguration_table(t *testing.T) {
func TestConfiguration_nextConfiguration_prevIndex(t *testing.T) {
// Stale prevIndex.
req := configurationChangeRequest{
command: AddStaging,
command: AddVoter,
serverID: ServerID("id1"),
serverAddress: ServerAddress("addr1"),
prevIndex: 1,
Expand All @@ -250,7 +260,7 @@ func TestConfiguration_nextConfiguration_prevIndex(t *testing.T) {

// Current prevIndex.
req = configurationChangeRequest{
command: AddStaging,
command: AddVoter,
serverID: ServerID("id2"),
serverAddress: ServerAddress("addr2"),
prevIndex: 2,
Expand All @@ -262,7 +272,7 @@ func TestConfiguration_nextConfiguration_prevIndex(t *testing.T) {

// Zero prevIndex.
req = configurationChangeRequest{
command: AddStaging,
command: AddVoter,
serverID: ServerID("id3"),
serverAddress: ServerAddress("addr3"),
prevIndex: 0,
Expand Down
4 changes: 1 addition & 3 deletions raft.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1370,9 +1370,7 @@ func (r *Raft) appendEntries(rpc RPC, a *AppendEntriesRequest) {
return
}
if entry.Term != storeEntry.Term {
r.logger.Warn("clearing log suffix",
"from", entry.Index,
"to", lastLogIdx)
r.logger.Warn("clearing log suffix", "from", entry.Index, "to", lastLogIdx)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated cleanup I had done while reading the code. This log line fits pretty easily on one line.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen this done stylistically to better show the pairing of variadic args (which I think reads better but this one is short enough not to be confusing)

if err := r.logs.DeleteRange(entry.Index, lastLogIdx); err != nil {
r.logger.Error("failed to clear log suffix", "error", err)
return
Expand Down