search

tikv / raft-rs

Raft distributed consensus algorithm implemented in Rust.
Apache License 2.0
2.86k stars 391 forks source link

`handle_committed_entries` after saving hard state in examples #492

Open e-ivkov opened 1 year ago

e-ivkov commented 1 year ago

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 commented 1 year ago

We faced an issue with this in our own project, you can check it here - https://github.com/qdrant/qdrant/pull/1172

BusyJay commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.