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.

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

Handle ErrCompacted errors for Compact in raftexample #16423

Conversation

chenyang8094
Copy link
Contributor

When I adjust the value of defaultSnapshotCount to be smaller than snapshotCatchUpEntriesN in raftexample, because the compactIndex cannot be updated, a panic will occur when the snapshot is triggered for the second time, and the error is "panic: requested index is unavailable due to compaction".

This PR adds ErrCompacted error handling for Compact (keep consistent with etcd server).

@chenyang8094 chenyang8094 force-pushed the fix-raftexample-panic-when-reduce-snapshot-count branch from a133deb to 8cf15c7 Compare August 16, 2023 05:45
@fuweid
Copy link
Member

fuweid commented Aug 16, 2023

@chenyang8094 Thanks for the patch. For the DCO requirement, you need to git commit -s --amend --no-edit && git push

@chenyang8094 chenyang8094 force-pushed the fix-raftexample-panic-when-reduce-snapshot-count branch from 8cf15c7 to c28eb3c Compare August 16, 2023 05:48
Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

Hey @chenyang8094 - Thanks for proposing this change.

Please be aware we are planning to remove contrib/raftexample from etcd.

There is an issue etcd-io/raft#2 to implement a self-contained raft example in the raft repo which has been worked on.

I'm not necessarily opposed to merging this, just making you aware of the other work underway. Will leave final merge decision to maintainers.

@chenyang8094
Copy link
Contributor Author

chenyang8094 commented Aug 16, 2023

@jmhbnz Thanks for the tip, I see it. IIUC, there seems to be a lot more to this work. raftexample is a great code for learning etcd/raft, let's improve it together.

@chenyang8094
Copy link
Contributor Author

@jmhbnz Can you answer my doubts, thank you. #16425

@jmhbnz
Copy link
Member

jmhbnz commented Aug 18, 2023

@jmhbnz Can you answer my doubts, thank you. #16425

Hey @chenyang8094 as mentioned above, I am not opposed to merging this, the code change looks ok to me, however it is now up to maintainers if they agree. @ahrtr WDYT?

@chenyang8094
Copy link
Contributor Author

chenyang8094 commented Aug 18, 2023

@jmhbnz I mean my Q&A, please click this link #16425

@chenyang8094
Copy link
Contributor Author

@jmhbnz @ahrtr
Please help me take a look at this Static Analysis failure, what am I doing wrong?

@ahrtr
Copy link
Member

ahrtr commented Aug 18, 2023

Please gofmt the source file

@chenyang8094 chenyang8094 force-pushed the fix-raftexample-panic-when-reduce-snapshot-count branch from 04a39b9 to e6027df Compare August 18, 2023 08:24
@ahrtr
Copy link
Member

ahrtr commented Aug 18, 2023

Could you squash the commits?

Signed-off-by: Chen Yang <[email protected]>
Signed-off-by: chenyangyang.cy <[email protected]>
@chenyang8094 chenyang8094 force-pushed the fix-raftexample-panic-when-reduce-snapshot-count branch from e6027df to 68db708 Compare August 18, 2023 12:05
@chenyang8094
Copy link
Contributor Author

Could you squash the commits?

done

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM.

It matchs how etcd processes the compact.

if err == raft.ErrCompacted {
return
}

@ahrtr ahrtr merged commit 39aad38 into etcd-io:main Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants