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

Has push model been considered for discovery API? #475

Closed
yurishkuro opened this issue Mar 3, 2017 · 5 comments
Closed

Has push model been considered for discovery API? #475

yurishkuro opened this issue Mar 3, 2017 · 5 comments

Comments

@yurishkuro
Copy link
Contributor

We have an internal service discovery that supports server push when service instances are added/removed. The current Subscriber interface is inherently pull-based, which works with the assumption that load balancer always asks for available endpoints for every RPC call, but that's not how some RPC frameworks are implemented, e.g. in TChannel I can update the Peers in the connection object, but connection never asks for them itself. Currently the only way to use sd.Subscriber with such connection is to have some bg loop that polls it periodically.

If I am not mistaken, the sd.zk.Subscriber is also based on the push model internally, so it would be good to expose push model via the top level interface.

@yurishkuro yurishkuro changed the title Have discovery API with push model been considered? Has push model been considered for discovery API? Mar 3, 2017
@peterbourgon
Copy link
Member

#209

@peterbourgon
Copy link
Member

I made a spike once, but it didn't work out, for reasons that escape me right now. I'd be happy to take a PR. It's important that the interface and its behaviors are kept consistent across implementations.

@yurishkuro
Copy link
Contributor Author

yurishkuro commented Mar 3, 2017

@peterbourgon I can try doing a PR. However, there are a few points we should agree on first.

  1. Discovery should deal with string instances, not Endpoints (per Can the discovery API be de-coupled from the Endpoint? #463). I think it's especially important for the push model to separate the notion of instance location (the discovery concern) from what to do with that location (e.g. creating Endpoint, an RPC concern).

  2. Should the push model be optional, since not all implementations can support it? It is possible to implement push via periodic pull (with a generic component), but I think it should be left to composition rather than requiring each implementation to support push.

type Discoverer interface {
   Instances() []string
}

type Notifier interface {
   Register(chan<- []string)
   Unregister(chan<- []string)
}

@yurishkuro
Copy link
Contributor Author

Did some prototyping, have to go with chan<- because functions are not comparable in Go, thus cannot implement Unregister. Updated previous comment.

@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

2 participants