Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can the discovery API be de-coupled from the Endpoint? #463

Closed
yurishkuro opened this issue Feb 11, 2017 · 11 comments
Closed

Can the discovery API be de-coupled from the Endpoint? #463

yurishkuro opened this issue Feb 11, 2017 · 11 comments

Comments

@yurishkuro
Copy link
Contributor

yurishkuro commented Feb 11, 2017

This is more of a wish list item, but let me at least describe the use case. In go-kit the discovery API is defined in terms of Endpoints, which can be used directly to make RPC calls. However, there are cases when I want to use just the discovery, i.e. finding the locations of other services, because I need to use some other APIs to call those services. Two concrete examples:

  1. my service talks to a Cassandra cluster. I could use discovery API during start-up of my service to find the seed Cassandra nodes that I can give to gocql driver.
  2. two of my services are communicating via TChannel, which is designed to balance the load internally against a list of peers, and the peers can be configured and changed dynamically, e.g. via updates from the discovery service.

It is not impossible to solve these two use cases with go-kit. If the Endpoint was an interface, I could implement it with a special struct that stores the string location of the remote service, and the actual Call function would be no-op. Then I could downcast the Endpoints returned from discovery to this struct and use it in the two above use cases. Given that the Endpoint is not an interface, I could still solve this by implementing the call function to return the struct with the location of the service.

Both of these approaches wouldn't be needed if the discovery API was defined in terms of service locations, as a basic discovery layer, while the current API could've been a wrapper around it for people who could use the Endpoints for RPC calls directly.

@peterbourgon
Copy link
Member

peterbourgon commented Feb 13, 2017

I'm open to a PR for your second approach.

Out of curiosity, how would you model an Endpoint as an interface?

type Endpoint interface {
    Invoke(ctx context.Context, request interface{}) (response interface{}, err error)
}

?

@willfaught
Copy link
Contributor

willfaught commented Feb 13, 2017 via email

@basvanbeek
Copy link
Member

ServeEndpoint could confuse people as the method would be called in both client and server endpoints. Invoke seems much more appropriate.

@yurishkuro
Copy link
Contributor Author

@peterbourgon

I'm open to a PR for your second approach.

Do you mean A PR for the actual separation of discovery API from the Endpoint? I can give it a try, but it's likely to be a breaking change.

@peterbourgon
Copy link
Member

I mean

I could … implement the call function to return the struct with the location of the service.

I understand it would be a breaking change—hopefully just within package sd? I'm mostly interested to see it sketched out, so I can better understand what the impact would be.

@yurishkuro
Copy link
Contributor Author

yurishkuro commented Feb 14, 2017

I could … implement the call function to return the struct with the location of the service.

So this isn't a breaking change, it's a hack that can be done without changing go-kit, e.g. like this:

type EndpointLocationRequest struct {}

// EndpointWithLocationFactory decorates a real factory by handling a special
// request EndpointLocationRequest and returning the instance/location as a string.
func EndpointWithLocationFactory(factory Factory) Factory {
    return func(instance string) (endpoint.Endpoint, io.Closer, error) {
        endpoint, closer, err := factory(instance)
        return func(ctx context.Context, request interface{}) (response interface{}, err error) {
            if _, ok := request.(EndpointLocationRequest); ok {
              return instance, nil // return location
            }
            return endpoint(ctx, request) // delegate to real endpoint
        }, closer, err
    }
}

@peterbourgon
Copy link
Member

Ah, okay. Yeah, I'd be curious to see what a non-hacky version would look like :)

@yurishkuro
Copy link
Contributor Author

A non-hacky, but somewhat API breaking version would be to introduce an intermediary interface:

type Discoverer interface {
	Instances() ([]string, error)
}

Currently implementations such as consul.NewSubscriber accept a Factory argument. Instead those constructors can be renamed consul.NewDiscoverer (does not need a Factory) and then a Subscriber can be created out of that generically:

type naiveSubscriber struct {
    factory       sd.Factory
    discoverer sd.Discoverer
}

func (s *naiveSubscriber) Endpoints() ([]endpoint.Endpoint, error) {
    // use factory to create endpoints from instances returned by the discoverer
    // in practice there would be some form of caching for previously created endpoints
}

This way we basically de-couple the discovery function from the function of creating and maintaining Endpoints.

@yurishkuro
Copy link
Contributor Author

@peterbourgon any thoughts?

@peterbourgon
Copy link
Member

I want to give this a proper read-through, but I won't have time for it for another week or two. Without going into the details, however, I like what I see so far; I'd be happy to take a look at a more complete API.

@peterbourgon
Copy link
Member

I guess this is done now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants