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_committed_entries after saving hard state in examples #492

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

e-ivkov
Copy link

@e-ivkov e-ivkov commented Oct 25, 2022

Changes

handle_committed_entries after saving hard state in examples.

Reason

Projects might base their consensus thread structure looking at these examples. In production environment most of the projects will not use in memory storage, but will save states on disk. And it is very important to save applied index increase only after the committed index (in hard state) was saved. Otherwise on restart this precondition might be broken, if program stopped in between the lines saving applied index and hard state. In that case Raft lib will fail with panic.

@e-ivkov
Copy link
Author

e-ivkov commented Oct 25, 2022

We faced an issue with this in our own project, you can check it here - qdrant/qdrant#1172

@BusyJay
Copy link
Member

BusyJay commented Oct 26, 2022

It's true that somehow the apply index should not be larger than the committed index. But it's not required to persist all hard state before applying entries. If all entries are persisted before being applied (which is the default behavior), the only consistency is just commit index instead of the whole hard state. In this case, it's safe to just update the commit index to max(hs.commit, applied) if you are sure the inconsistency is an expected data loss under race (for example, apply entries is written before hard state). This flexibility can improve latency.

All in all, I agree the behavior needs to be better documented, but I don't think it's necessary to always require hard state being persisted before handling committed entries.

@e-ivkov
Copy link
Author

e-ivkov commented Oct 26, 2022

I agree that it can be handled in other ways, I guess my main point is about mentioning this in documentation.

If you can suggest where to add it, I think I can do this!

@BusyJay
Copy link
Member

BusyJay commented Oct 27, 2022

Turns out it's already stated in the docs:

Note, although Raft guarentees only persisted committed entries will be applied, but it doesn’t guarentee commit index is persisted before being applied. For example, if application is restarted after applying committed entries before persisting commit index, apply index can be larger than commit index and cause panic. To solve the problem, persisting commit index with or before applying entries. You can also always assign commit index to the max(commit_index, applied_index) after restarting, it may work but potential log loss may also be ignored silently.

https://docs.rs/raft/latest/raft/

I think this also worths mentioned in the example just right before handling committed entries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants