etcdserver: record removed member to check incoming message #1343
Conversation
| @@ -116,10 +117,26 @@ func nodeToMember(n *store.NodeExtern) (Member, error) { | |||
| // Remove removes a member from the store. | |||
| // The given id MUST exist. | |||
| func (s *clusterStore) Remove(id uint64) { | |||
| p := s.Get().FindID(id).storeKey() | |||
| if _, err := s.Store.Delete(p, true, true); err != nil { | |||
| if _, err := s.Store.Delete(Member{ID: id}.storeKey(), true, true); err != nil { | |||
There was a problem hiding this comment.
memberStoreKey(id uint64)? so it is consistent with removedMemberStoreKey
There was a problem hiding this comment.
more than glad to do that.
c3f5564 to
386b619
Compare
|
have addressed all comments. |
| if err := h.server.Process(context.TODO(), m); err != nil { | ||
| log.Println("etcdhttp: error processing raft message:", err) | ||
| writeError(w, err) | ||
| switch err { | ||
| case etcdserver.ErrRemoved: |
There was a problem hiding this comment.
Sorry for not being clear previously. here is what i want:
// Validate validates the incoming messages.
if err := h.server.Validate(m); err != nil {
...
}
// stats logic
// process logic|
lgtm. actually i do not have a strong opinion on fixing the |
| @@ -43,6 +43,7 @@ type ClusterStore interface { | |||
| Add(m Member) | |||
| Get() Cluster | |||
| Remove(id uint64) | |||
| IsRemoved(id uint64) bool | |||
There was a problem hiding this comment.
The naming in the interface is getting messy:
probably this is cleaner
GetCluster() Cluster
GetMember() Member
AddMember(m Member)
RemoveMember(id uint64)
IsRemovedMember(id uint64) boolThere was a problem hiding this comment.
It is kind of confused now..
let us settle this down after clusterstore becomes stable.
|
let us do validation stuff in the follow ups. |
| log.Panicf("delete peer should never fail: %v", err) | ||
| } | ||
| if _, err := s.Store.Create(removedMemberStoreKey(id), false, "", false, store.Permanent); err != nil { | ||
| log.Panicf("unexpected creating removed member error: %v", err) |
There was a problem hiding this comment.
this is awkward, how about just "creating RemovedMember should never fail: %v"
|
lgtm |
etcdserver: record removed member to check incoming message
I put removed member in a new directory other than members because 1. removed members has no need to record raftAttributes and attributes anymore 2. removed members have different use case from active members, and they don't need to be fetched at the same time