From 14e1442d2d4a4dec515e7ca5a81381a6c2e50b66 Mon Sep 17 00:00:00 2001 From: Jonathan Boulle Date: Fri, 7 Nov 2014 10:57:42 -0800 Subject: [PATCH] etcdserver: sort IDs and s/getIDset/getIDs/ --- etcdserver/force_cluster.go | 29 +++++++++++++++++++---------- etcdserver/force_cluster_test.go | 32 ++++++++++++++++---------------- 2 files changed, 35 insertions(+), 26 deletions(-) diff --git a/etcdserver/force_cluster.go b/etcdserver/force_cluster.go index 7b6c640af..d5f4064e0 100644 --- a/etcdserver/force_cluster.go +++ b/etcdserver/force_cluster.go @@ -18,6 +18,7 @@ package etcdserver import ( "log" + "sort" "github.com/coreos/etcd/pkg/pbutil" "github.com/coreos/etcd/pkg/types" @@ -43,7 +44,7 @@ func restartAsStandaloneNode(cfg *ServerConfig, index uint64, snapshot *raftpb.S } // force append the configuration change entries - toAppEnts := createConfigChangeEnts(getIDset(snapshot, ents), uint64(id), st.Term, st.Commit) + toAppEnts := createConfigChangeEnts(getIDs(snapshot, ents), uint64(id), st.Term, st.Commit) ents = append(ents, toAppEnts...) // force commit newly appended entries @@ -62,12 +63,12 @@ func restartAsStandaloneNode(cfg *ServerConfig, index uint64, snapshot *raftpb.S return } -// getIDset returns a set of IDs included in the given snapshot and the entries. -// The given snapshot contians a list of IDs. -// The given entries might contain two kinds of ID related entry. -// If the entry type is Add, the contained ID will be added into the set. -// If the entry type is Remove, the contained ID will be removed from the set. -func getIDset(snap *raftpb.Snapshot, ents []raftpb.Entry) map[uint64]bool { +// getIDs returns an ordered set of IDs included in the given snapshot and +// the entries. The given snapshot/entries can contain two kinds of +// ID-related entry: +// - ConfChangeAddNode, in which case the contained ID will be added into the set. +// - ConfChangeAddRemove, in which case the contained ID will be removed from the set. +func getIDs(snap *raftpb.Snapshot, ents []raftpb.Entry) []uint64 { ids := make(map[uint64]bool) if snap != nil { for _, id := range snap.Nodes { @@ -89,13 +90,21 @@ func getIDset(snap *raftpb.Snapshot, ents []raftpb.Entry) map[uint64]bool { log.Panicf("ConfChange Type should be either ConfChangeAddNode or ConfChangeRemoveNode!") } } - return ids + sids := make(types.Uint64Slice, 0) + for id := range ids { + sids = append(sids, id) + } + sort.Sort(sids) + return []uint64(sids) } -func createConfigChangeEnts(ids map[uint64]bool, self uint64, term, index uint64) []raftpb.Entry { +// createConfigChangeEnts creates a series of Raft entries (i.e. +// EntryConfChange) to remove the set of given IDs from the cluster. The ID +// `self` is _not_ removed, even if present in the set. +func createConfigChangeEnts(ids []uint64, self uint64, term, index uint64) []raftpb.Entry { ents := make([]raftpb.Entry, 0) next := index + 1 - for id := range ids { + for _, id := range ids { if id == self { continue } diff --git a/etcdserver/force_cluster_test.go b/etcdserver/force_cluster_test.go index 7b5d7beb4..f4673f857 100644 --- a/etcdserver/force_cluster_test.go +++ b/etcdserver/force_cluster_test.go @@ -24,7 +24,7 @@ import ( "github.com/coreos/etcd/raft/raftpb" ) -func TestGetIDset(t *testing.T) { +func TestGetIDs(t *testing.T) { addcc := &raftpb.ConfChange{Type: raftpb.ConfChangeAddNode, NodeID: 2} addEntry := raftpb.Entry{Type: raftpb.EntryConfChange, Data: pbutil.MustMarshal(addcc)} removecc := &raftpb.ConfChange{Type: raftpb.ConfChangeRemoveNode, NodeID: 2} @@ -35,20 +35,20 @@ func TestGetIDset(t *testing.T) { snap *raftpb.Snapshot ents []raftpb.Entry - widSet map[uint64]bool + widSet []uint64 }{ - {nil, []raftpb.Entry{}, map[uint64]bool{}}, - {&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{}, map[uint64]bool{1: true}}, - {&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{addEntry}, map[uint64]bool{1: true, 2: true}}, - {&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{addEntry, removeEntry}, map[uint64]bool{1: true}}, - {&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{addEntry, normalEntry}, map[uint64]bool{1: true, 2: true}}, - {&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{addEntry, removeEntry, normalEntry}, map[uint64]bool{1: true}}, + {nil, []raftpb.Entry{}, []uint64{}}, + {&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{}, []uint64{1}}, + {&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{addEntry}, []uint64{1, 2}}, + {&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{addEntry, removeEntry}, []uint64{1}}, + {&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{addEntry, normalEntry}, []uint64{1, 2}}, + {&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{addEntry, removeEntry, normalEntry}, []uint64{1}}, } for i, tt := range tests { - idSet := getIDset(tt.snap, tt.ents) + idSet := getIDs(tt.snap, tt.ents) if !reflect.DeepEqual(idSet, tt.widSet) { - t.Errorf("#%d: idset = %v, want %v", i, idSet, tt.widSet) + t.Errorf("#%d: idset = %#v, want %#v", i, idSet, tt.widSet) } } } @@ -57,35 +57,35 @@ func TestCreateConfigChangeEnts(t *testing.T) { removecc2 := &raftpb.ConfChange{Type: raftpb.ConfChangeRemoveNode, NodeID: 2} removecc3 := &raftpb.ConfChange{Type: raftpb.ConfChangeRemoveNode, NodeID: 3} tests := []struct { - ids map[uint64]bool + ids []uint64 self uint64 term, index uint64 wents []raftpb.Entry }{ { - map[uint64]bool{1: true}, + []uint64{1}, 1, 1, 1, []raftpb.Entry{}, }, { - map[uint64]bool{1: true, 2: true}, + []uint64{1, 2}, 1, 1, 1, []raftpb.Entry{{Term: 1, Index: 2, Type: raftpb.EntryConfChange, Data: pbutil.MustMarshal(removecc2)}}, }, { - map[uint64]bool{1: true, 2: true}, + []uint64{1, 2}, 1, 2, 2, []raftpb.Entry{{Term: 2, Index: 3, Type: raftpb.EntryConfChange, Data: pbutil.MustMarshal(removecc2)}}, }, { - map[uint64]bool{1: true, 2: true, 3: true}, + []uint64{1, 2, 3}, 1, 2, 2, @@ -95,7 +95,7 @@ func TestCreateConfigChangeEnts(t *testing.T) { }, }, { - map[uint64]bool{2: true, 3: true}, + []uint64{2, 3}, 2, 2, 2,