Zoned ServiceInstanceListSupplier #658
Conversation
OlgaMaciaszek
requested review from
ryanjbaxter,
spencergibb,
marcingrzejszczak and
Haybu
| * @return The {@link String} value from instance metadata corresponding to the | ||
| * {@link ServiceInstance#ZONE} key. | ||
| */ | ||
| default String getZone() { |
There was a problem hiding this comment.
I'd rather not pollute ServiceInstance with shortcuts for metadata. I think this can go safely in the ZonePreferenceServiceInstanceListSupplier
| zone = environment.getProperty("spring.cloud.loadbalancer.zone"); | ||
| } | ||
| if (zone != null) { | ||
| List<ServiceInstance> filteredInstances = serviceInstances.stream() |
There was a problem hiding this comment.
See @dsyer's comment #652 (comment) about stream(). Let's change to a loop here.
There was a problem hiding this comment.
Didn't realise Stream API had performance issues. Hope they improve as it's so much more readable to me. Will switch it here, however.
...a/org/springframework/cloud/loadbalancer/core/ZonePreferenceServiceInstanceListSupplier.java
Show resolved
Hide resolved
There was a problem hiding this comment.
This LGTM, but I do have one question.
I might be confusing concepts here, but with Ribbon I believe we always got all instances across all zones but if there was an instance within the same zone Ribbon would select that instance above the others (and only try the others if it failed and we retried). With this implementation if we only return instances within the same zone we would loose the functionality of trying services in another zone when retrying wouldn't we?
That's probably something to think about. Ribbon has a whole logic of verifying whether the instances are up and running. We, however, re-query the SR to get the running instances. In the cached version, this might be slightly affected, but reducing the TTL should work. We don't also really provide any retry support OOTB here, but it might be a good idea to add it. We could then do the first execution on a filtered subset and then retry on the entire set. I think we could provide sth like a |
|
Yes, it would be another issue |
Fixes gh-632