Discovery metadata

[jmnarloch]

This pull request introduces the metadata map that can be associated with each service instance registered within discovery service (currently covered by Eureka and Zookeper, Consul is planned to have such support in version 2.0) allowing to use that information on the client side.

I had to also align the version with latest spring boot snapshot where some class has been renamed, this is way there are two commits.

[jmnarloch]

Probably it's worth getting back to the issue despite the spring-cloud/spring-cloud-netflix#419 and #41

My idea is simply, if the backing discovery service can provide the client with the metadata (like Eureka and Zookeeper) we can cover that, for Consul it could return the tags information for version 1.x and when version 2.0 will be covered the metadata could be provided from there (issues: hashicorp/consul#1131, hashicorp/consul#697, hashicorp/consul#1107).

[spencergibb]

At a glance, I think I like this better than #41.

[jmnarloch]

The only question is whether a "static" Map would be sufficient and won't be a need to do some lookup on the fly and there therefor introduce some InstanceMetadata abstraction.

[spencergibb]

When you say, "static" map, do you mean just a java.util.Map?

[jmnarloch]

Yes, exactly. I was only deliberating whether introducing there would be a need to introduce some wrapper on top of the map, for instance if you could declare some interface (let's call if InstanceMetadata) then the implementation could have much more flexibility. Although I not sure whether there will be ever need for that, so for now I just propose simple key/value map for storing the metadata.

[spencergibb]

I think DefaultServiceInstance needs to retain the original constructor signature as well as the new one. Hadn't noticed this before.

[jmnarloch]

Done, thanks for noticing.

[spencergibb]

Thanks @jmnarloch!

[spencergibb]

@jmnarloch so I started looking at this. The two netflix implementations in EurekaDiscoveryClient were simple to implement. I ran into trouble in RibbonLoadBalancerClient. The ribbon Server doesn't even expose the underlying InstanceInfo where the metadata is located. Wondering if you had any thoughts?

[spencergibb]

I'd have to do something like this

    protected RibbonServer(String serviceId, Server server, boolean secure) {
        this.serviceId = serviceId;
        this.server = server;
        this.secure = secure;

        if (this.server instanceof DiscoveryEnabledServer) {
            Field instanceInfoField = ReflectionUtils.findField(DiscoveryEnabledServer.class, "instanceInfo");
            ReflectionUtils.makeAccessible(instanceInfoField);
            try {
                InstanceInfo instanceInfo = (InstanceInfo) instanceInfoField.get(this.server);
                this.metadata = instanceInfo.getMetadata();
            } catch (IllegalAccessException e) {
                log.error("Error getting instanceInfo for serviceId: "+serviceId, e);
            }
        }
    }

@dsyer @adriancole @marcingrzejszczak thoughts?

[spencergibb]

The above stinks because it won't work for s-c-zk and s-c-consul, which will use RibbonServer but not DiscoveryEnabledServer.

[spencergibb]

Actually we already have a ServerIntrospector abstraction we could add Map<String, String> getMetadata(Server server).

[spencergibb]

And there is a DiscoveryEnabledServer.getInstanceInfo method, so no reflection needed, I'm happy.

[spencergibb]

Closed with some polish via aa2ddce

[marcingrzejszczak]

The metadata feature is a very good idea. It can be combined together with the "dependency" approach that we have in Zookeeper.