Skip to content

Conversation

@smcvb
Copy link

@smcvb smcvb commented May 16, 2017

This PR is to solve #204, and introduces a void putMetadata(String key, String value) and void setMetadata(Map<String, String> metadata) function to the ServiceInstance interface.

This addition might prove interesting for other frameworks, which want to rely on spring-cloud-commons it's DiscoveryClient/ServiceInstance set up to do service discovery, but also want to adjust the ServiceInstance.metadata without the need of knowing which Spring Cloud implementation is used.

smcvb added 3 commits May 16, 2017 14:38
…ceInstance

Introduce putMetadata(String key, String value) and
 setMetadata(Map<String, String> metadata) to ServiceInstance, to be
 able to adjust the metadata field through the ServiceInstance api.
Implement putMetadata(String key, String value) and
 setMetadata(Map<String, String> metadata) in DefaultServiceInstance to
 comply with the interface adjustment.
The setMetadata() will be somewhat lengthy as we are dealing with a
 final map.
Implement putMetadata(String key, String value) and
 setMetadata(Map<String, String> metadata) in SimpleServiceInstance.
}

@Override
public void setMetadata(Map<String, String> metadata) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the approach of removing all the old fields and adding them because metadata is final for this class.

Removing final from the field seemed more problematic because of the @RequiredArgsConstructor.

@smcvb smcvb changed the title #204 Introduce putMetadata() and setMetadata() function to ServiceInstance gh-204 Introduce putMetadata() and setMetadata() function to ServiceInstance May 16, 2017
@spencergibb
Copy link
Member

Wouldn't you be able to do all of this, assuming the impl returned a read/write version of the metadata?

@smcvb
Copy link
Author

smcvb commented Sep 11, 2017

@spencergibb yes I'd say so. That was my initial assumption of the getMetadata() function too.
But after trying out the Eureka and Consul implementation of the DiscoveryClient / ServiceInstance I found out this did hold for Eureka, but didn't for Consul.

Hence the idea to add a set()/put() function to the interface.

smcvb added 2 commits December 4, 2017 16:51
# Conflicts:
#	spring-cloud-commons/src/main/java/org/springframework/cloud/client/DefaultServiceInstance.java
Reorder functions a little

[spring-cloud#204]
@smcvb
Copy link
Author

smcvb commented Dec 4, 2017

Updated this PR to point to align with the current master branch.
@spencergibb am I still required to provide specific ServiceInstance implementations for all specific version like Consul/Eureka?

After fixing the build obviously.

@spencergibb
Copy link
Member

Since we're on Java 8, create empty default methods on the interface

smcvb added 2 commits December 5, 2017 10:40
Add empty default functionality for put and set metadata

[spring-cloud#204]
-Set constructor parameters on one line
-Add javadoc for 'instance' parameter

[spring-cloud#204]
@smcvb
Copy link
Author

smcvb commented Dec 5, 2017

Added empty default methods per suggestion.

@smcvb
Copy link
Author

smcvb commented Dec 18, 2017

@spencergibb is this PR still being hold back by something you'd like me to do?
Or is @dsyer his comment on #204 such that we should debate whether this should still go in or not?

@spencergibb
Copy link
Member

Closing, if we were to support something like this, it would be with a mutable backing map, but I have my doubts even for that #204 (comment)

@smcvb smcvb deleted the issue204 branch April 25, 2018 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants