Allow updating a service registration metadata

[smcvb]

Would it be possible to have some form of put() or set() function for the Map<String,String> metadata in the ServiceInstance?
I'd like to be able to not rely on a specific Spring Cloud implementation for adjusting the metadata (like through Eureka's properties, or the tags in Consul), but just use the api in spring-cloud-commons to update a ServiceIntance it's metadata.

This could for example give the possibility for other frameworks to set framework-specific knowledge in the ServiceInstance.metadata field and not having to know anything of specific implementation. Thus, only needing the spring-cloud-commons dependency.

[ryanjbaxter]

It doesn't seem unreasonable to me. @spencergibb what do you think?

[spencergibb]

It sounds reasonable, getting something that works across impls might prove tricky.

[ryanjbaxter]

@smcvb want to submit a PR?

[smcvb]

@ryanjbaxter sure thing!
Just a change to the ServiceInstance interface to add a setMetadata(Map) and putMetadata(key, value) I'd guess?

[ryanjbaxter]

That seems OK. Just make sure you also update the implementations as well.

[smcvb]

@ryanjbaxter did you also mean for me to adjust the specific Spring Cloud ServiceInstance implementations by Eureka/Consul/etc, or just the implementations in spring-cloud-commons?

[ryanjbaxter]

@smcvb all of the implementations will need to be changed.

[smcvb]

Ok, I'll see what time I can free up to help with that �

[dsyer]

I don't really like putMetaData() much, since all it does is mirror the call to the underlying map. We might as well just document that the map is mutable and make sure that all interfaces support that somehow (or break in an informative way).

[spencergibb]

@dsyer, not all impls have a simple map backing them (consul in particular).

[dsyer]

I guess that’s why I said “break in an informative way�. But if it can support the putValue method, I’d say it can support a mutable backing map.

[spencergibb]

After looking at consul, ServiceInstance is not used by consul during registration. Updating metadata there means nothing. It happens to work in eureka, since the backing object happens to be one eureka uses and sends deltas during heartbeats.

There is no consistent way to update a registration in spring cloud. I'm now positive we don't want #209 and I might go so far to say that the contract of getMetadata() should be adjusted to indicate an unmodifiable map.

[spencergibb]

I think the issue is: support updating a registration, whether that is metadata or something else. Only certain registration systems could support this by default (eureka), others might need work (consul, zookeeper) by responding to an event when metadata has changed.

[salaboy]

@spencergibb, @dsyer, et all I was thinking more in the lines of auto-wiring a bean like what is possible in eureka:

@Autowired    
  private ApplicationInfoManager aim; 
...
  aim.setMetadata(....); 

If we replace ApplicationInfoManager for our interface in spring cloud common, then writing a simple wrapper that delegate to each provider will do the work.
I can create a PR to support eureka and spring-cloud-k8s discovery, but I will need help on zookeper and consul.
Does this make sense?

[spencergibb]

Why do you need another bean besides a configuration properties bean?

[salaboy]

@spencergibb a bean to programatically add or change those properties at runtime based on a set of conditions.
At the end of the day this is just a "delegate" to the underlaying implementation that will do a remote call to the real registry.
So instead of:

@Autowired     private ApplicationInfoManager aim;

which is what people is using with Eureka, I want to have one in spring cloud commons that can delegate to ApplicationInfoManager when you have eureka client in the class path
as well to a different implementation. Something like:

@Autowired     private ServiceMetadataManager smm;

Notice that in k8s this will change the service itself, and not the individual spring boot application instances (Pods). There is a considerable mismatch in that area between k8s and spring cloud.
If this doesn't make too much sense, we can start with a simpler approach. I will appreciate feedback so I can allocate some time to work on this.

[spencergibb]

@salaboy We've talked about adding methods to the ServiceRegistry interface to update metadata since ServiceInstance is a return type from DiscoveryClient. Since s-c-k8s doesn't have a ServiceRegistry, maybe that functionality should be specific to s-c-k8s.

[salaboy]

@spencergibb I think that is starting to make sense to me. We can add it in the KubernetesServiceRegistry class as well, which is not used by kubernetes but it can add that functionality. As soon as we can do

@Autowired
private ServiceRegistry serviceRegistry;
serviceRegistry.addMetaData(Map<String, Object> metadata);

Sounds good to me..
Is this what you have in mind?

[spencergibb]

@salaboy I wouldn't create a ServiceRegistry unless you are really going to register services, not just mutate metadata.