Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More simplifications in fabric8 discovery implementation #1403

Merged
merged 40 commits into from
Aug 8, 2023

Conversation

wind57
Copy link
Contributor

@wind57 wind57 commented Aug 7, 2023

No description provided.

@wind57 wind57 changed the title More simplifications More simplifications in fabric8 discovery implementation Aug 7, 2023
@@ -59,41 +59,41 @@ private DiscoveryClientUtils() {
* - service type
* </pre>
*/
public static Map<String, String> serviceMetadata(String serviceId, Map<String, String> serviceLabels,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. we are modifying a public method here, but one which was never released, it was introduced a few commits back. That is one of the reasons I did not start this work before, but only after the previous release, so that I have enough time to potentially change method signatures, after some clean-up.

  2. What this change does is encapsulate 3 fields:

  • serviceId (service name)
  • serviceLabels
  • serviceAnnotations

into a single record : ServiceMetadata. This class already existed under a different name, also introduced recently and not released yet.

}

public static ServicePortNameAndNumber endpointsPort(LinkedHashMap<String, Integer> endpointsPorts,
String serviceId, KubernetesDiscoveryProperties properties, Map<String, String> serviceLabels) {
ServiceMetadata serviceMetadata, KubernetesDiscoveryProperties properties) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here, instead of

  • serviceId
  • Map<String, String> serviceLabels

pass a single ServiceMetadata instance

*
* @author wind57
*
*/
public record ServiceMetadataForServiceInstance(String name, Map<String, String> labels,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rename a public record not yet released + add two more fields to it.


List<EndpointAddress> addresses = addresses(endpointSubset, properties);
for (EndpointAddress endpointAddress : addresses) {

ServiceMetadataForServiceInstance forServiceInstance = forServiceInstance(service);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here is one improvement that I did not notice until the clean-up was made. This line:

ServiceMetadataForServiceInstance forServiceInstance = forServiceInstance(service);

was computed in a loop of :

for (EndpointSubset endpointSubset : subsets) {

though it's only needed once. So I extracted it before it:

ServiceMetadata serviceMetadata = serviceMetadata(service);

Otherwise the changes here are related to renaming the ServiceMetadata record.

@wind57 wind57 marked this pull request as ready for review August 8, 2023 08:25
@wind57
Copy link
Contributor Author

wind57 commented Aug 8, 2023

@ryanjbaxter this is ready to go too. thank you.

@ryanjbaxter ryanjbaxter added this to the 3.0.5 milestone Aug 8, 2023
@ryanjbaxter ryanjbaxter merged commit c046499 into spring-cloud:3.0.x Aug 8, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants