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

raft: error on ignored proposal and wait for EntryConfChange results #44

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+6 −13
Draft

raft: error on ignored proposal and wait for EntryConfChange results #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ func (n *node) ProposeConfChange(ctx context.Context, cc pb.ConfChangeI) error {
if err != nil {
return err
}
return n.Step(ctx, msg)
return n.stepWait(ctx, msg)
}

func (n *node) step(ctx context.Context, m pb.Message) error {
Expand Down
2 changes: 1 addition & 1 deletion raft.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1229,7 +1229,7 @@ func stepLeader(r *raft, m pb.Message) error {

if refused != "" {
r.logger.Infof("%x ignoring conf change %v at config %s: %s", r.id, cc, r.prs.Config, refused)
m.Entries[i] = pb.Entry{Type: pb.EntryNormal}
Copy link
Member

@fuweid fuweid Apr 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Switching to normal entry is trying to increase applied index so that the next confChange can be applicable. It seems that it is right move to return dropped if we have a way to update pendingConfIndex.

Copy link
Member

@chaochn47 chaochn47 Apr 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Switching to normal entry is trying to increase applied index so that the next confChange can be applicable.

I don't think switching to normal entry will make a difference here to increase applied index. The advance of applied index is triggered by etcd. But I agree returning proposal dropped early is helpful.

Copy link
Member

Choose a reason for hiding this comment

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

You are right. The raft will update applied index even if the apply fails. I was thinking that the normal is used to catch up with pendingConfIndex so that it won't impact the following confChange.

Copy link
Member

Choose a reason for hiding this comment

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

aha. Thanks for explanation! I think next ProposeConfChange() will succeed once the raft.Applied is advanced by etcd raftNode r.Advance()

Copy link
Author

Choose a reason for hiding this comment

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

For reference, the use of normal entry was added here
I think the idea was to support mixed list of entries (ConfChanges and Normal) in proposal, but is it even possible to get mixed list right now?
When I check for creation of pb.Message{Type: pb.MsgProp, there are two distinct places:

  1. regular Propose
  2. confChangeToMsg - used in ProposeConfChange and raft.appliedTo

return ErrProposalDropped
Copy link
Member

Choose a reason for hiding this comment

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

I don't know the side effect of dropping other entries in the same m.Entries for loop.

How about moving it til the end of stepLeader function, especially after bcastAppend so other entries handling are not impacted just to be safe?

Copy link
Author

Choose a reason for hiding this comment

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

the loop is only processing config changes, see if cc != nil {. So, I think it's ok to drop here. Also, my understanding is that we have to drop full proposal, there is no per entry ErrProposalDropped.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks for the explanation!

} else {
r.pendingConfIndex = r.raftLog.lastIndex() + uint64(i) + 1
}
Expand Down
15 changes: 4 additions & 11 deletions raft_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3283,24 +3283,17 @@ func TestStepConfig(t *testing.T) {
}

// TestStepIgnoreConfig tests that if raft step the second msgProp in
// EntryConfChange type when the first one is uncommitted, the node will set
// the proposal to noop and keep its original state.
// EntryConfChange type when the first one is uncommitted, the node will drop second msgProp and return ErrProposalDropped.
func TestStepIgnoreConfig(t *testing.T) {
// a raft that cannot make progress
r := newTestRaft(1, 10, 1, newTestMemoryStorage(withPeers(1, 2)))
r.becomeCandidate()
r.becomeLeader()
r.Step(pb.Message{From: 1, To: 1, Type: pb.MsgProp, Entries: []pb.Entry{{Type: pb.EntryConfChange}}})
index := r.raftLog.lastIndex()
pendingConfIndex := r.pendingConfIndex
r.Step(pb.Message{From: 1, To: 1, Type: pb.MsgProp, Entries: []pb.Entry{{Type: pb.EntryConfChange}}})
wents := []pb.Entry{{Type: pb.EntryNormal, Term: 1, Index: 3, Data: nil}}
ents, err := r.raftLog.entries(index+1, noLimit)
if err != nil {
t.Fatalf("unexpected error %v", err)
}
if !reflect.DeepEqual(ents, wents) {
t.Errorf("ents = %+v, want %+v", ents, wents)
err := r.Step(pb.Message{From: 1, To: 1, Type: pb.MsgProp, Entries: []pb.Entry{{Type: pb.EntryConfChange}}})
if err != ErrProposalDropped {
t.Fatalf("Expected %v, got %v", ErrProposalDropped, err)
}
if r.pendingConfIndex != pendingConfIndex {
t.Errorf("pendingConfIndex = %d, want %d", r.pendingConfIndex, pendingConfIndex)
Expand Down