Introduce builder pattern for server initialization

[jcdyer]

I'm working on a fork of raft-rs, where I'm hoping to implement some missing raft features including persistent logs, snapshotting, and cluster reconfiguration.

As I was getting started, I noticed that a lot of the configuration was hardcoded, and thought I'd give a shot at making it more extensible by introducing a ServerBuilder. This way, new features can be added without constantly changing the public signature for creation.

I also tried to disentangle the server creation and event loop creation. The envisioned interface is demoed in the example code.

Let me know if this is a direction you're interested in pursuing, or if there are any modifications you'd like to see.

[Hoverbear]

Hey! :)

Thanks for submitting this!

I really like the builder idea. I have to admit I'm not a huge fan of builders which .expect() if they are missing required arguments. How do you feel about making it so the initial .builder() taking the required arguments, and the builder methods being used for optionals?

Disentangling the Raft server from the event loop is a goal I'd like to pursue as well. @skade and I have been discussing a way to implement Raft using futures and then building a way to attach it to a Tokio core.

[arthurprs]

I can help reviewing if you want to send more PRs this way.

[Hoverbear]

Thanks, @arthurprs !

[jcdyer]

How do you feel about making it so the initial .builder() taking the required arguments, and the builder methods being used for optionals?

I like that idea. It leaves less guesswork to the caller, and places more correctness on the type system.

The current implementation has the interface you're asking for (I believe), but still uses Options internally, because I'm not sure how to handle passing the ownership of the non-clonable StateMachine otherwise. This lets me pass it over without having to create an object to replace it, but the expect should now be irrefutable. I made the finalize method consume the builder, so it can't get reused erroneously.

[Hoverbear]

So what you can do is actually make finalize consume self instead of just borrow it:

pub struct ServerBuilder<L, M>
where
    L: Log,
    M: StateMachine,
{
    id: ServerId,
    addr: SocketAddr,
    peers: HashMap<ServerId, SocketAddr>,
    store: L,
    state_machine: M,
    max_connections: usize,
    election_min_millis: u64,
    election_max_millis: u64,
    heartbeat_millis: u64,
}

impl <L, M> ServerBuilder<L, M>
where
    L: Log,
    M: StateMachine
{
    fn new(id: ServerId, addr: SocketAddr, peers: HashMap<ServerId, SocketAddr>, store: L, state_machine: M) -> ServerBuilder<L, M> {
        /// Create a ServerBuilder with default values
        /// for optional members.
        ServerBuilder {
            id: id,
            addr: addr,
            peers: peers,
            store: store,
            state_machine: state_machine,
            max_connections: 128,
            election_min_millis: 150,
            election_max_millis: 350,
            heartbeat_millis: 60,
        }
    }

    pub fn finalize(self) -> Result<Server<L, M>> {
        Server::create(
            self.id,
            self.addr,
            self.peers,
            self.store,
            self.state_machine,
            self.election_min_millis,
            self.election_max_millis,
            self.heartbeat_millis,
            self.max_connections,
        )
    }

At this point, due to how the server works when you .run(), the need for a separate builder seems rather low. Since Server is, in a sense, finalized by using .run(). Maybe we could put the builder methods directly on Server instead?

[jcdyer]

I took out the need for an extra .finalize() call, by adding a ServerBuilder::run() that calls back into Server::run(). I kept the separate builder, though, for a few reasons:

• The state it contains is represented differently on the Server struct.
• Keeping the builder better captures the semantics of "configure, then run." Configuration methods shouldn't be called on a running server, and this makes that impossible.
• It keeps the code more cleanly organized, as configuration code is all in one place, and server behavior in another.

[arthurprs]

I wouldn't block the PR over this but I think only the absolutely required show be in new. So maybe peers should be optional in with_peers.

[Hoverbear]

I would say I generally agree with @arthurprs .

Otherwise, the interaction/interplay between ServerBuilder and Server feels a bit awkward and weird to me... But that's partially due to the code being already a bit of a mess. I'm OK with merging this under the intention of cleaning things up more as we go. :)

Let me know if we're good for merge or there is something you'd like to touch up/address.

[jcdyer]

I'm happy to clean up the peering set up before we merge. I'll give it a try tonight. I'm thinking either an "add_peer(ServerId, SocketAddr)" or an "add_peers(Iterator<(ServerId, SocketAddr)>)" (or IntoIterator<..>)

[jcdyer]

Ok. I think this is good to go. I added a with_peers method, though it only takes a HashMap for now. I think I'll be able to make it work with an IntoIterator, but I need to figure out a little more about how to use the relevant traits. That can come in another PR. I also renamed Server::create to Server::finalize to match what the builder is doing.

[jcdyer]

If you give it a thumbs up, I'll squash the commits into one before you merge.

[Hoverbear]

I squashed them. :)

[jcdyer]

Awesome!

[Hoverbear]

[jcdyer]

Keytar Bear: The finest street musician in Boston.