-
Notifications
You must be signed in to change notification settings - Fork 689
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 DelegatingServiceInstanceListSupplier must #1226
Fix DelegatingServiceInstanceListSupplier must #1226
Conversation
respect SelectedInstanceCallback on its delegate. Motivation behind is that some load balancer implementations like the RoundRobinLoadBalancer to call selectedServiceInstance on the ServiceInstanceListSupplier after choosing the next instance. However, when a ServiceInstanceListSupplier in wrapped by the DelegatingServiceInstanceListSupplier this functionality breaks, as DelegatingServiceInstanceListSupplier is not implementing SelectedInstanceCallback and therfore does ignoring that its delegate might also do. This might break the functionality of the delegate. Fixes spring-cloudgh-1221.
@spencergibb @OlgaMaciaszek anybody? |
Thanks for providing the PR, @HJK181. Will take a look at it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @HJK181, thanks for submitting the PR. It looks good, but there are some cosmetic changes that need to be made - please address the comments. Also, please update the forst line of the copyright clause on top of each changed file to -2023
.
.../java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java
Outdated
Show resolved
Hide resolved
...st/java/org/springframework/cloud/loadbalancer/core/TestSelectedServiceInstanceSupplier.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @HJK181 . Have added one more comment. Please take a look.
.../java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java
Show resolved
Hide resolved
d2ddb6a
to
5b24a9b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @HJK181. LGTM.
respect
SelectedInstanceCallback
on its delegate. Motivation behind is that some load balancer implementations like theRoundRobinLoadBalancer
to callselectedServiceInstance
on theServiceInstanceListSupplier
after choosing the next instance. However, when aServiceInstanceListSupplier
in wrapped by theDelegatingServiceInstanceListSupplier
this functionality breaks, asDelegatingServiceInstanceListSupplier
is not implementingSelectedInstanceCallback
and therefore does ignoring that its delegate might also do. This might break the functionality of the delegate. Fixes gh-1221.