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

Fix ReactiveDiscoveryClient probe() method default implementation #1105

Closed
trajano opened this issue May 7, 2022 · 6 comments · Fixed by #1201
Closed

Fix ReactiveDiscoveryClient probe() method default implementation #1105

trajano opened this issue May 7, 2022 · 6 comments · Fixed by #1201
Assignees
Labels
Milestone

Comments

@trajano
Copy link

trajano commented May 7, 2022

Describe the bug

the .probe method is defined as

default void probe() {
    getServices();
}

and getServices() also returns a Flux, since Flux don't do anything without a subscribe this is practically a no-op

Perhaps the interface should be defined as returning Mono<Void> as in

default Mono<Void> probe() {
    return getServices()
      .then();
}
@OlgaMaciaszek
Copy link
Collaborator

Hi @trajano, thanks for reporting it. This is right, however, it's actually done on purpose. The assumption is that the concrete implementations should override it, for example: https://github.com/spring-cloud/spring-cloud-consul/blob/main/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryClient.java#L104 .

@trajano
Copy link
Author

trajano commented Jan 19, 2023

The ConsulDiscoveryClient is not a ReactiveDiscoveryClient though.

@OlgaMaciaszek
Copy link
Collaborator

Right, that was probably not the best example. There are implementations of ReactiveDiscoveryClient in various projects, such as SimpleReactiveDiscoveryClient and SimpleReactiveDiscoveryClient and that is where appropriate implementation should be added.

@trajano
Copy link
Author

trajano commented Jan 24, 2023

Okay but if something like https://github.com/spring-cloud/spring-cloud-commons/blob/main/spring-cloud-commons/src/main/java/org/springframework/cloud/client/discovery/simple/reactive/SimpleReactiveDiscoveryClient.java did implement it with the current signature, what would it mean?

Because it's return value is not a Mono<void> but a void

@OlgaMaciaszek
Copy link
Collaborator

Thanks, @trajano - that's right. Makes sense. Will mark it as a bug.

@OlgaMaciaszek OlgaMaciaszek changed the title Implement probe() in SimpleReactiveDiscoveryClient Fix ReactiveDiscoveryClient probe() method default implementation Jan 25, 2023
@OlgaMaciaszek OlgaMaciaszek self-assigned this Jan 25, 2023
@trajano
Copy link
Author

trajano commented Jan 25, 2023

That change would likely trigger a lot of problems especially for those that made custom implementations already.
I would recommend at least just mark the existing method as deprecated first and have a default implementation like

default Mono<Void> reactiveProbe() {
  return Mono.fromRunnable(this::probe);
}

@OlgaMaciaszek OlgaMaciaszek added this to the 3.1.6 milestone Jan 30, 2023
@OlgaMaciaszek OlgaMaciaszek linked a pull request Feb 1, 2023 that will close this issue
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 a pull request may close this issue.

3 participants