DiscoveryClient API comments

[aantoniadis]

I have been using the library recently and I have a couple of comments regarding the API that might worth taken into consideration. io.dropwizard.discovery.client.DiscoveryClient exposes the following methods

/**
 * Return the running instances for the service.
 * 
 * @return Collection of service instances
 */
public Collection<ServiceInstance<T>> getInstances(
        @Nonnull final String serviceName) throws Exception {
    return discovery.queryForInstances(serviceName);
}

/**
 * Return a cached list of the running instances for the service.
 * 
 * @return Collection of service instances
 */
public Collection<ServiceInstance<T>> getInstances() {
    return cache.getInstances();
}

/**
 * Return an instance of this service.
 * 
 * @return ServiceInstance
 * @throws Exception
 */
public ServiceInstance<T> getInstance() throws Exception {
    return provider.getInstance();
}

My comment is that it is not intuitive whether a call on a method will return results from the cache or not. Maybe if we used different names for each case it would make the intention of each method much clearer.

Also, since the DiscoveryClient client is create by DiscoveryClient.newDiscoveryClient where the serviceName is passed. Why would somebody call DiscoveryClient#Collection<ServiceInstance<T>> getInstances(@Nonnull final String serviceName)? From the factory API, I would expect the discoveryClient to find 1 type of service.

@jplock what do you think?

[jplock]

Those were mostly exposed as helper methods to access ZK. Since I initially created this module, I've never actually used it in production, so if there are improvements or cleanups we can make, I'd be all for that.

[aantoniadis]

@jplock So how is the DiscoveryClient supposed to be used by the services? I would expect it to be used to retrieve the location of required service and pass it to an httpClient. Maybe we should include such an example in the README.md.

If so, I believe that is would be useful to create a client that does this discovery (and possible retries) under the hood and provide a fluent interface (something like https://github.com/Netflix/ribbon).

[jplock]

@aantoniadis I used Ribbon in my dropwizard-consul implementation I'm maintaining at https://github.com/smoketurner/dropwizard-consul/tree/master/consul-ribbon

[aantoniadis]

@jplock any plans to merge dropwizard-cosul with the current project?

[jplock]

@aantoniadis probably not

[sliu2013]

@jplock Just want to ensure I understand the above comment correctly.... So this repo "dropwizard-discovery" is mainly for doing service registration to zookeeper?! As for service discovery client, you recommend to use "dropwizard-consul" instead? Or maybe apache curator?

[jplock]

This library will register a service with ZK and includes a client to get a host and port from ZK if you want to talk to another service. This library doesn’t include a load balancer implementation like dropwizard-consul does.

[sliu2013]

This DiscoveryClient is the client that I can use to get host/port from ZK, right?

[jplock]

@sliu2013 correct