relab/gorums

Make RPC and Unicast be methods on a single node configuration?

meling opened this issue · 4 comments

When thinking about certain aspects related to Node vs Configuration, I came to think about the following. Should we just drop support for invoking methods on a Node, and instead only allow invoking a method on a Configuration?? I'm thinking about our RPC and unicast call types that both work directly on a Node type. Here is what the code would look like:

// GRPCCall plain gRPC call; testing that Gorums can ignore these, but that
// they are added to the _grpc.pb.go generated file.
func (c *Configuration) GRPCCall(ctx context.Context, in *Request) (resp *Response, err error) {
	if c.Size() != 1 {
		return nil, fmt.Errorf("cannot invoke gRPC call on configuration with size %d", c.Size())
	}
	cd := gorums.CallData{
		Node:    c.Nodes()[0],
		Message: in,
		Method:  "dev.ZorumsService.GRPCCall",
	}

Instead of:

func (n *Node) GRPCCall(ctx context.Context, in *Request) (resp *Response, err error) {
	cd := gorums.CallData{
		Node:    n.Node,
		Message: in,
		Method:  "dev.ZorumsService.GRPCCall",
	}

Reason: We can avoid the following struct in the generated code, and things become easier to maintain (we need one less translation layer...):

type Node struct {
	*gorums.Node
}

@Raytar and @leandernikolaus Thoughts?

Would could add a helper method to the Configuration type to return a single node Configuration object from an existing configuration.

I prefer to have separate types for Node and Configuration. It doesn't make sense to me that a "unicast" method would be implemented on a configuration of nodes.

Would it be possible to get rid of the Configuration struct instead? Maybe we could use type Configuration []*Node?

Edit: The main problem with this idea is where to put the QSpec.

Not done think about this, but in #116, there are now both dev.Configuration and gorums.Configuration, and QSpec is only defined on dev.Configuration.

I've also realized that we could define func (n *Node) GRPCCall( ... ) in package gorums instead of making the Node object part of the CallData type. Similarly, other call types defined in gorums can now be defined as func (c *Configuration) QuorumCall( ... ), instead of making Nodes []*gorums.Node be part of the QuorumCallData type. Not sure if it is better or not, but we have the possibility to do it.

We will keep both Node and Configuration.