Skip to content

Commit

Permalink
Change member removal logic to remove members only once. (#4254)
Browse files Browse the repository at this point in the history
Currently, after removing a member, the logs print an error saying the
member cannot be removed because it's not part of the group. This
happens because the node tries to remove members more than once. It
succeeds the first time but subsequent attempts will fail.

This PR adds a check so that the node is not removed if it's already
been marked as removed in the old state and is not a current peer of the
node.

I tested this by adding logs, removing a node, and verifying the node is
only removed once and further updates don't try to remove the node
again.
  • Loading branch information
martinmr committed Nov 13, 2019
1 parent 5ddc142 commit 717710a
Showing 1 changed file with 17 additions and 3 deletions.
20 changes: 17 additions & 3 deletions worker/groups.go
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ func (g *groupi) applyState(state *pb.MembershipState) {
if g.state != nil && g.state.Counter > state.Counter {
return
}
oldState := g.state
g.state = state

// Sometimes this can cause us to lose latest tablet info, but that shouldn't cause any issues.
Expand Down Expand Up @@ -324,11 +325,24 @@ func (g *groupi) applyState(state *pb.MembershipState) {
if g.Node != nil {
// Lets have this block before the one that adds the new members, else we may end up
// removing a freshly added node.
for _, member := range g.state.Removed {
if member.GroupId == g.Node.gid && g.Node.AmLeader() {

for _, member := range g.state.GetRemoved() {
if member.GetGroupId() == g.Node.gid && g.Node.AmLeader() {
go func() {
// Don't try to remove a member if it's already marked as removed in
// the membership state and is not a current peer of the node.
_, isPeer := g.Node.Peer(member.GetId())
// isPeer should only be true if the rmeoved node is not the same as this node.
isPeer = isPeer && member.GetId() != g.Node.RaftContext.Id

for _, oldMember := range oldState.GetRemoved() {
if oldMember.GetId() == member.GetId() && !isPeer {
return
}
}

if err := g.Node.ProposePeerRemoval(
context.Background(), member.Id); err != nil {
context.Background(), member.GetId()); err != nil {
glog.Errorf("Error while proposing node removal: %+v", err)
}
}()
Expand Down

0 comments on commit 717710a

Please sign in to comment.