elixir-toniq / raft

An Elixir implementation of the raft consensus protocol
Apache License 2.0
427 stars 29 forks source link

Default names for peers #5

Closed keathley closed 6 years ago

keathley commented 6 years ago

Right now we need to specify a unique name for each peer in the cluster. We also need to include these names when we bootstrap a cluster by running set_configuration. I'm wondering if we could continue to allow users to specify names but provide a default name like {__MODULE__, node()}. The __MODULE__ in this case would be the user defined state machine module. To make this more concrete lets say a use has a module like:

defmodule KVStore do
  use Raft.StateMachine
end

We could provide a start_peer function in so that a user could simply call KVStore.start_peer() and the implementation would look something like:

quote do
  def start_peer(opts \\ []) do
    opts = Keyword.put_new(opts, :name, {unquote(__MODULE__), unquote(node())})
    Raft.start_peer(unquote(__MODULE__), opts)
  end
end

This would also allow us to change the api for adding and removing nodes. If we default to the module name then the only configuration we need to require is something like Raft.set_configuration(leader, [:a@mymachine, :b@mymachine, :c@mymachine]).

I need to think through all of the implications of this but interested to see what others think.

bitwalker commented 6 years ago

You'd want unquote(__CALLER__.module) rather than unquote(__MODULE__), and you don't want to expand the call to node(), but that wasn't your question :P

I'm not sure I follow on the set_configuration bit, are you saying it would be a macro which expands to something effectively like Raft.really_set_configuration(leader, [{KVStore, :"a@mymachine"}, ...])?

It might instead make more sense to have set_configuration take 3 arguments, the leader, the Raft module (i.e. KVStore in your example), and then the node list, and internally apply the Raft module to each element of the node list. This would be equivalent to saying "build a Raft cluster with members named KVStore running on nodes A, B, and C". Then you take care of joining the name with the node to form the tuple for communicating with those processes. Since the Raft module is a key part of the API, I'm not sure hiding it will always be viable, it is probably desirable to make things explicit, but make the API convenient enough, that you don't have to do something like wrap every node name in a tuple with the Raft module. Things get tricky though if you want to support some kind of process registry using {:via, registry, name}, because then you need to explicitly provide those tuples as elements of the list; but you could support a hybrid list, where plain atoms are paired with the module name to get the {name, node} pair, and anything else is treated as-is, so that you could either use the convenient API, or provide a list of via tuples if that's what you need. It still feels less than perfect, but I'm mostly spitballing ideas here.

I don't yet have a strong opinion on this though until I have a chance to do some experimentation, but just conceptually this is my two cents, hope that is the feedback you were looking for :)

keathley commented 6 years ago

Stepping back, the whole reason I wanted to consider this was to make it easier to specify clusters, add new peers, etc. I'm not sure that this implementation is going to really satisfy that goal in the best way. Its probably better to see how #4 pans out and then evaluate after that.

keathley commented 6 years ago

Closing this for now because I think this issue will be mitigated in #4.