Fix a bug where the first log appended is not Idx=1 #24
Conversation
There was a problem hiding this comment.
🚀
I have a test comment but nothing blocking.
I have a more general question (maybe we should discuss this in different circonstances, so feel free to disregard but I will document it here before I forget 😅).
I see that the raft-wal library is dependent to our raft library, do you think that this dependency is really needed or we can transition to remove it at some point? If it's needed and the raft-wal is meant for being used with our raft lib, what is the rationale of having it as a separate library?
Interesting question @dhiaayachi. I didn't see that as a downside of keeping it separate. Today we have to depend on Raft in all our LogStore implementations because part of the interface includes passing in a I don't think that's a bad pattern and I don't think it means we should move this code into Raft - having this separate from BoltDB and other implementations makes sense to me. And third-parties should be able to implement this in just the same way and would also need to pull in the base lib to do so. There will never be a circular dependency as long as raft lib itself never needs to pull in specific implementations which it doesn't because it can test with stubs. If we ever want integration tests in the raft lib against a set of our "supported" implementations, we can do that as a separate package from raft lib itself and avoid a circular dep that way. I think of it as being like how if you write an HTTP "middleware" in Go, you have to import Does that make sense or is there some downside I'm not thinking of of depending on raft here? |
This is an alternative approach to fix the issue in #23 after I realised that there were a few complicated problems not addressed there.
This came out of realising that Raft's behaviour on snapshot restores is to "leave a hole" in the log which is not tolerated by WAL. Instead we have a PR for Raft that truncates all current logs and starts appending again but that uncovered this issue.
The WAL layer actually had test for this case, however they assumed it could be fixed by tracking the meta data differently, meanwhile the actual segment layer assumed BaseIndex was always really the actual first log index and so broke when that is violated.
This fix makes WAL actually maintain that invariant always (and tests for it in all cases) so instead it has to recreate segment with the correct BaseIndex if it's empty and then next Append is not the expected next BaseIndex.
I want to also write some integration tests for WAL with full raft cluster to exercise all the raft code paths that interact with LogStore and ensure there are no other bad assumptions. But that will probably be a separate PR at least due to size. This changes and additional testing already improves on the coverage around this specific issue.