willemt / raft

C implementation of the Raft Consensus protocol, BSD licensed
Other
1.13k stars 271 forks source link

Log interface decoupling #82

Open yossigo opened 6 years ago

yossigo commented 6 years ago

Here's a major refactoring step to consider.

Currently the server logic and log implementation are quite coupled, to the point where the log implementation in many cases is responsible to trigger operations on the Raft side.

Decoupling would not just clean things up, but also make it possible to replace the log implementation. For example, it may be useful to implement a disk based log that does not require all entries to be in memory at all time and can fetch them from disk on demand.

liw commented 6 years ago

log's interface (and perhaps also callbacks) could be "closer" to what server wants. For instance, "get the term of an entry", "get a reference to a range of entries, including their data", "put a reference to a range of entries", etc. That would allow us to remove the circular buffer implementation and users to implement entry caching and referencing according their specific storage (possibly using hash tables, trees, etc., provided by other libraries).

freeekanayaka commented 6 years ago

@liw what you describe is indeed very close to the design of the log store interface exposed by hashicorp's raft implementation (in Go).

I'm porting a raft-based application from Go to C and not requiring all logs to be in memory all the time would be precisely the use case I have at hand.

willemt commented 6 years ago

I like the idea of decoupling the log implementation a lot.

I would first like to finish "batchifying" the log entry API (https://github.com/willemt/raft/pull/51). This is because I don't want to specify a log entry API interface which will change immediately once we add batching APIs.

yossigo commented 6 years ago

There is already quite a lot of work in progress on this: https://github.com/yossigo/raft/blob/03212d48950054993d20480b8b7cab50be7ec4db/include/raft.h#L410

Some observations and ideas moving forward:

  1. The log interface above is already (mostly) capable of batch processing entries. It's not really enough, because: a) Also core Raft callbacks need to be adapted. b) In the real world batching won't be enough to improve performance, and some form of async work will be needed to avoid the Raft main loop blocking on I/O.
  2. The existing in-memory log implemented by raft_log.c is wrapped and usable as default internal implementation. Callbacks that are Log-related rather than Raft-related have been split from raft_cbs to raft_log_cbs, and are used by the raft_log.c implementation directly to maintain compatibility.
  3. Most but not all invocations of Log -> Raft have been removed, this is still in progress. There is no real reason for them anyway.
  4. This is also a good opportunity to address ownership of raft_entry_t passing around, possibly reference counting and a custom free function.
yossigo commented 5 years ago

@willemt this turned out to be a bigger deal than expected, with API changes leaking elsewhere but I think it's good progress. There's a branch that seems quite stable, tests passing etc.

https://github.com/yossigo/raft/tree/feature/pluggable-log-impl