handle_committed_entries after saving hard state in examples#492
handle_committed_entries after saving hard state in examples#492e-ivkov wants to merge 1 commit intotikv:masterfrom
handle_committed_entries after saving hard state in examples#492Conversation
Signed-off-by: Egor Ivkov <e.o.ivkov@gmail.com>
d689d7e to 3673722 Compare | We faced an issue with this in our own project, you can check it here - qdrant/qdrant#1172 |
| 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. |
| 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! |
| Turns out it's already stated in the docs:
https://docs.rs/raft/latest/raft/ I think this also worths mentioned in the example just right before handling committed entries. |
Changes
handle_committed_entriesafter 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.