Refactor `raftexample` using interfaces to better separate concerns #1

This is taken straight from github.com/etcd-io/etcd:client/pkg/fileutil/lock_flock.go Signed-off-by: Michael Haggerty <[email protected]>

Signed-off-by: Michael Haggerty <[email protected]>

This interface defines what `kvstore` needs for saving and loading snapshots. Signed-off-by: Michael Haggerty <[email protected]>

This interface defines what `httpKVAPI` needs as the backing key-value store. Signed-off-by: Michael Haggerty <[email protected]>

Initialize the snapshot storage in `newRaftNode()` rather than in `startRaft()`. This is a step towards getting rid of the `snapshotStorageReady` channel. Signed-off-by: Michael Haggerty <[email protected]>

This is another step towards getting rid of `snapshotStorageReady`. Signed-off-by: Michael Haggerty <[email protected]>

Rename `newRaftNode()` to `startRaftNode()`, and change it to replay the WAL synchronously, before returning. This doesn't change anything, because the callers were all waiting for `snapshotStorageReady` before proceeding anyway. Signed-off-by: Michael Haggerty <[email protected]>

Use a local variable (in `startRaftNode()`) instead. Signed-off-by: Michael Haggerty <[email protected]>

This is more consistent with the types used elsewhere for IDs. Signed-off-by: Michael Haggerty <[email protected]>

This is the sort of thing that the caller should be able to inject, and it's not part of the raft algorithm itself. This change makes it clearer what a user of the raft algorithm must supply to it. Signed-off-by: Michael Haggerty <[email protected]>

Extract two new methods, for clarity and to reduce code duplication. Signed-off-by: Michael Haggerty <[email protected]>

Change `newKVStore()` to set up the initial state, but not start the `readCommits()` goroutine anymore. Instead, change the callers to call `processCommits()` (a renamed version of `readCommits()`) themselves (in a goroutine). The main benefit of this change is that the `kvstore` can be instantiated before calling `startRaftNode()`, meaning that `startRaftNode()` can be passed `kvs.getSnapshot` as an argument. Previously, it had to be passed an anonymous function that refers to a variable (`kvs`) that has not yet been initialized. This asynchrony was confusing and unnecessary. Signed-off-by: Michael Haggerty <[email protected]>

It wasn't doing anything useful. Signed-off-by: Michael Haggerty <[email protected]>

Add a new interface, `FSM`, which represents a finite state machine that can be driven by raft. Also add `kvfsm`, which is an `FSM` view of a `kvstore`. Treating `kvstore` and `kvfsm` as separate types means that the public interface of `kvstore` is not contaminated with internal methods that are only there to support raft and should not be invoked by the user. Signed-off-by: Michael Haggerty <[email protected]>

Signed-off-by: Michael Haggerty <[email protected]>

Make `cluster` hold an array of `peer`s, rather than one array for each peer attribute. Continue storing the names separately, since this `[]string` has to be passed to `startRaftNode()`. Signed-off-by: Michael Haggerty <[email protected]>

The tests can be made cleverer if each `peer` in a `cluster` can be instantiated with an arbitrary finite state machine. We'll use this to make some tests able to operate more like normal clients of `raftNode`s. Signed-off-by: Michael Haggerty <[email protected]>

Signed-off-by: Michael Haggerty <[email protected]>

Handle such errors at the caller, instead. Signed-off-by: Michael Haggerty <[email protected]>

This is a step towards moving `ProcessCommits()` out of this interface and into `raftNode`. Signed-off-by: Michael Haggerty <[email protected]>

Instead, make `LoadAndApplySnapshot()` part of the `FSM` interface, and have the callers of `newKVStore()` invoke that method after calling `newKVStore()`. This is a step towards moving this functionality entirely out of `FSM`. Signed-off-by: Michael Haggerty <[email protected]>

Now that the `FSM` interface is more capable, there is no reason that this method, which has more to do with raft anyway, can't be implemented in `raftNode`. This makes it easier to implement new FSMs without having to reimplement this method. This means that `newRaftNode()` has to return a pointer to the `raftNode`, so make that change, too. This change will make other future changes simpler, too. Signed-off-by: Michael Haggerty <[email protected]>

Make this a standard part of starting up a node. Signed-off-by: Michael Haggerty <[email protected]>

This is the last use of `kvstore.snapshotStorage`, so remove that field as well. Signed-off-by: Michael Haggerty <[email protected]>

The old method, monitoring the `errorC` channel, is not great because the error only pops out of that channel once. Which of the pieces of code that are waiting on that channel reads the error? Nobody knows! Instead, provide a `Done()` method and an `Err()` method that interested parties can use to determine when the node finishes and what error it returned. The callers haven't been changed yet. Signed-off-by: Michael Haggerty <[email protected]>

Use the `done` channel instead of the `errorC` channel for monitoring for the completion of the raft node. If the channel is closed, don't log the error, since the caller of `ProcessCommits()` is already taking care of that. Since this means that we're not killing the server by aborting the program, we now have to call `srv.Close()` explicitly. Signed-off-by: Michael Haggerty <[email protected]>

Signed-off-by: Michael Haggerty <[email protected]>

Instead of driving the channels within the test, user a cleverer FSM and call `ProcessCommits()` to manage the channels. The old version of the test only looked at the first data item in each `commit`, even though multiple data items can be sent in a single `commit`. It also only looked at the first 100 commits, even though there is a multiplication factor because each node initiates 100 proposals. The new version checks all data items, and checks that the total number of commits is as expected (namely, 100 for each node, or 300 total). Signed-off-by: Michael Haggerty <[email protected]>

Keep `commitC` and `errorC` internal to `raftNode`: don't return them from `newRaftNode()`, and don't require them as arguments to `(*raftNode).ProcessCommits()`. (Some tests still need them, but they can access the members directly on the `raftNode` instance.) Signed-off-by: Michael Haggerty <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `raftexample` using interfaces to better separate concerns #1

Refactor `raftexample` using interfaces to better separate concerns #1

Commits on Jul 19, 2023

Refactor raftexample using interfaces to better separate concerns #1

Are you sure you want to change the base?

Refactor raftexample using interfaces to better separate concerns #1

Commits on Jul 19, 2023

Refactor `raftexample` using interfaces to better separate concerns #1

Refactor `raftexample` using interfaces to better separate concerns #1