Refactor contrib/raftexample using interfaces to better separate concerns

[mhagger]

I come to etcd by way of evaluating go.etcd.io/raft for use in a project. When I saw that there was a sample application raftexample showing how to use the module, I started reading it, hoping that this would make it obvious what components I would have to implement to use raft in my own application.

But unfortunately, it wasn't easy to figure that out from the source code of raftexample. True, it provides a nice end-to-end example. Also, raftNode is a fairly nice abstraction, showing how to deal with the channels going into and coming out of the raft library. But I didn't find the interface between raftNode and the "application code" super transparent. For example,

• It is not obvious what features the Snapshotter must provide.
• It's hard to see the boundary between the raft code and the finite state machine being driven by it. kvstore is the FSM, but it also does things like read straight out of raft-managed channels and interact directly with the snapshotter.
• httpKVAPI contains a pointer to the whole kvstore, even though it really only interacts with its "public" interface, Propose() and Lookup().
• The design forces an awkward initialization step, where main() creates a getSnaphost function that closes over an initialized variable (kvs) then passes that function into newRaftNode(). That function returns a shapshotterReady <-chan *snap.Snapshotter channel from which main() reads a Snapshotter. It was unclear how much of this was essential complexity and how much could be avoided. (It turns out that most of this complexity can be avoided.)
• newRaftNode() returns commitC, which is handed on to newKVStore() and read from a goroutine in that type. It seems like it would be simpler for raftNode to read from commitC and invoke kvstore methods to trigger commits.
• newRaftNode() returns errorC, to which it writes an error if there is a problem. But multiple pieces of code try to read from that channel, and the semantics of Go channels make it indeterminate which one will discover that there was an error. (In general, error reporting is somewhat inconsistent.)

This PR consists of a series of refactorings that attempt to fix a bunch of the problems listed above:

• It introduces a few interfaces and uses them to better separate concerns:

  • SnapshotStorage represents permanent storage for snapshots. It only needs to know how to read and write them:

    type SnapshotStorage interface {
        SaveSnap(snapshot raftpb.Snapshot) error
        Load() (*raftpb.Snapshot, error)
        LoadNewestAvailable(walSnaps []walpb.Snapshot) (*raftpb.Snapshot, error)
    }
    

  • FSM represents the finite state machine that raftNode is able to drive. It only has to know how to take and restore snapshots and to apply commits:

    type FSM interface {
        TakeSnapshot() ([]byte, error)
        RestoreSnapshot(snapshot []byte) error
        ApplyCommits(commit *commit) error
    }
    

  • KVStore represents the interface that httpKVAPI uses to interact with the key-value store. Note that this interface only exposes application-level concepts and doesn't really expose anything about raft, but internally it has to know how to serialize its calls into log entries that can be managed by raft:

    type KVStore interface {
        Propose(k, v string)
        Lookup(k string) (string, bool)
    }
    

• kvstore now presents a KVStore face to httpKVAPI and a FSM face to raftNode, but itself is now a passive type (no internal goroutines).

• Now raftNode implements more of the raft data flows internally; in particular, its ProcessCommits() method reads commits from commitC and applies them to the kvstore via the FSM interface.

• Now main() creates the snapshotter and passes it to raftNode. This eliminates the complexity of snapshotterReady.

• raftNode now has Done() and Err() methods (replacing the errorC channel) that the caller can use to find out when it is done and whether it experienced any errors.

I think these changes make the code easier to read, and more importantly, make it easier for the reader to figure out would they would have to change to use the raft module within their own application, even if that application doesn't happen to be a key-value store.

This is a long patch series, but each commit is a self-contained step, meant to be readable/reviewable independently, and the tests pass after each commit. I'd be happy to break it up into multiple smaller PRs if that is preferable.

Along the way I fixed a couple of other minor problems, such as the ambiguity about error reporting mentioned above, and in the strange TestProposeOnCommit() test. See the commit messages for more information.

I only changed code within contrib/raftexample. But within that package, I have changed some internal interfaces. I assume that since this is just demonstration code, there are no backwards-compatibility promises made about this package. For the same reason, I have introduced some interfaces where there were none before, which will probably decrease performance very slightly and maybe force a few more allocations. Again, my goal was to make the code easier to understand, and I assume that preserving uncompromising performance is a lower priority than clarity.

I think that some further refactoring would be nice, to better delineate the interfaces between raftNode and the write-ahead log, and perhaps between raftNode and the transport layer. But I'll wait to see how this PR is received before proceeding.

I hope this is helpful!

[ahrtr]

Thanks @mhagger for the PR.

Actually we are planning to remove contrib/raftexample from etcd. Please see the comments below,

• raft: decouple raft release from etcd release #14713 (comment)
• raft: decouple raft release from etcd release #14713 (comment)

There is an issue etcd-io/raft#2 to implement a self-contained raft example in the raft repo. Probably you can discuss with @Elbehery on how to integrate your PR with his (not ready yet).

[Elbehery]

Thanks @ahrtr 👍🏽

@mhagger Hello ✋🏽 .. I am also based in Berlin 😄

I have dropped you a msg on k8s Slack

Looking forward

[mhagger]

@ahrtr: thanks for the info and the link.

I'm actually happy that raftexample is being moved out of the etcd repo. I was kindof wondering why it was there in the first place, aside from the convenience of "borrowing" dependencies from etcd. But that heavy use of etcd dependencies, IMHO, makes raftexample less useful as a pedagogical demonstration of how to use the raft library.

I think it would be attractive for raftexample to have its own barebones implementations of a WAL and a snapshotter, maybe even just using naive and non-performant (but correct!) filesystem calls for example. It might also be nice for it to offer industrial-strength implementations of those concepts, too, for people who don't want to customize them for their own application. But let these components be hidden behind a Go interface to make them easier to swap out.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

[mhagger]

Closing. I implemented the same refactoring in Elbehery/raft#1 against the raft repo. That PR supersedes this one.