Support service tags in ServiceInstance

[avthart]

Tags are a list of values that can be used to distinguish between "master" or "slave" nodes, different versions, management port, or any other service level labels.

Are there already any plans to support this or can I help?

[spencergibb]

@avthart pull requests are welcome. This will be tricky as different implementations might or might not have support tags. Fits well with consul :-) X-ref https://github.com/spring-cloud/spring-cloud-consul/issues/19

[mbenson]

Since there is a published Java 7 API, and because tags are (at least currently) not relevant to Eureka service discovery, it's probably a good idea to explore design options before any contributor goes off half-cocked only to have his approach rejected.

One straightforward-seeming approach is to create a TaggedServiceInstance subinterface. This is fairly brittle when one imagines future one-off features that are supported in this implementation library and not that one. Worse, to avoid ugly instanceof checks and casts, the instinctive or "obvious" approach is to create a TaggedDiscoveryClient interface, but the design of spring-cloud is such that this would also suggest a TaggedLoadBalancerClient and IMHO this exceeds the threshold of ridiculousness. Similarly it doesn't make much sense to declare a public Consul-specific API whether it would define interfaces or concrete classes.

It is still desirable to model the "tagged" (or any optional/additional) behavior in an interface, but I don't think it's particularly important to have ConsulServiceInstance implements TaggedServiceInstance vs. ConsulServiceInstance implements ServiceInstance, Tagged. If Tagged were a standalone interface, it could perhaps be a subtype of a ServiceInstanceBehavior interface, which could be interesting as we continue down this train of thought, e.g.:

public interface ServiceInstanceBehavior {
}

public interface Tagged extends ServiceInstanceBehavior {
    List<String> getTags();
}

Unfortunately I can't see what to do further that wouldn't require instanceof checks* at some point, until spring-cloud-commons moves to Java 8 and can benignly modify the ServiceInstance interface.

* or equivalent calls on java.lang.Class

[spencergibb]

Since we control the api there's no reason to use instanceof checks. I could see something like boolean hasCapability(TAGS) or hasTagCapability() or something similar.

[mbenson]

By "since we control the api" do you mean to say that it is kosher to add methods to a public interface within spring-cloud?

As for your API suggestions, #hasCapability(TAGS) looks more scalable, but then what does the method look like to retrieve the data associated with some capability?

<T> T getData(Capability capability);

Where each member of enum Capability documents the associated type and callers use type inference? Conversely, explicit methods for each prospective capability would be less messy from the perspective of type-safety, but proliferation would pollute the ServiceInstance API.

[mbenson]

@spencergibb: Fleshing out the scalable enum Capability approach, I have created https://github.com/mbenson/spring-cloud-commons/commit/192ca3807e0c92b1fdce9bf28c6a0ea0ebba04b8 and https://github.com/mbenson/spring-cloud-consul/commit/59843c1d89cbaed0ce9bc22ab5e1a9b3934a7d32 . WDYT?

[spencergibb]

That looks good enough for pull requests.

[kevinconaway]

@mbenson Should Capability be represented as an enum?

Doesn't that tie the underlying discovery implementations (consul, eureka etc) to the available types in that enum?

If Consul wanted to add more capabilities, the implementor would have to then update spring-cloud-commons first before proceeding.

[mbenson]

I think it's nice to make concepts available in some typesafe manner. I originally thought about declaring a marker ServiceCapability interface that could be implemented by the Capability enum to allow the use of types beyond the enum, but the resulting code didn't really seem that much superior to what's been done here. Ultimately, there needs to be some reliable means for the "capability consumer" and the DiscoveryClient to refer to a given capability, and an enum satisfies that requirement admirably.

[kevinconaway]

. I originally thought about declaring a marker ServiceCapability interface that could be implemented by the Capability enum to allow the use of types beyond the enum, but the resulting code didn't really seem that much superior to what's been done here.

In my view, using a marker interface decouples the underlying capability implementations from this parent library. With an enum, spring-cloud-commons would need to change every time one of the underlying service discovery implementations wants to add a new capability.

Ultimately, there needs to be some reliable means for the "capability consumer" and the DiscoveryClient to refer to a given capability

Wouldn't it be up the the underlying implementation to provide that type safety? Users know whether they are interacting with Consul, Eureka or whatever the underlying service discovery framework is

[mbenson]

What happens when multiple DiscoveryClients implement the same capability?

[mbenson]

If you're going to use something specific to the DiscoveryClient's implementation, why use the commons abstraction at all?

[kevinconaway]

What happens when multiple DiscoveryClients implement the same capability?

They would both have a similar capability, I would imagine. I'd say the duplication is a trade-off but I'm not in the best position to weigh those merits

If you're going to use something specific to the DiscoveryClient's implementation, why use the commons abstraction at all?

Fair point. In my case, there are other features in spring-cloud-consul that make it worthwhile to use.

All that said, it seems like you've thought this through already and are comfortable with your decision. Thanks for talking me through it.

[mbenson]

Well, when all is said and done, I'm just part of the peanut gallery. @spencergibb any thoughts on where the whole issue stands?

[spencergibb]

@mbenson sorry for the delay in responding. I can see https://github.com/spring-cloud/spring-cloud-consul/issues/48 working without the abstraction here in commons. If you want to search by tags, cast to ConsulDiscoveryClient and be on your way.