Respect discovery client order for reactive instance lookup - #1739
Respect discovery client order for reactive instance lookup#1739hutiefang76 wants to merge 1 commit into
Conversation
Try discovery publishers in configured order, falling back only when a publisher completes empty. Preserve every instance from the first non-empty client without subscribing to lower-priority publishers. Fixes spring-cloudgh-1542 Signed-off-by: hutiefang76 <137664623+hutiefang76@users.noreply.github.com>
| Flux<ServiceInstance> serviceInstances = Flux.empty(); | ||
| for (ReactiveDiscoveryClient discoveryClient : discoveryClients) { | ||
| serviceInstances.add(discoveryClient.getInstances(serviceId)); | ||
| serviceInstances = serviceInstances.switchIfEmpty(discoveryClient.getInstances(serviceId)); |
There was a problem hiding this comment.
What happens if the user now wants the previous behavior? For example it might be desirable in many cases to move to the next priority client if it returns and the higher priority client is still has not responded. I don't necessarily think that would be a bad thing.
There was a problem hiding this comment.
That is a valid compatibility concern. With this patch, a slow or non-terminating higher-priority publisher delays or prevents fallback; the old race can return a lower-priority client's result in that situation. Applications may rely on that latency behavior even though the client list is sorted.
I suggest keeping the existing fastest-non-empty behavior as the default and making strict priority ordering opt-in, with tests for both modes. That would address #1542 without changing existing applications by default. Would a configuration property on the reactive composite auto-configuration be the preferred way to expose this, or would you rather keep it as a constructor-level option? I'll align the API choice before extending the patch.
3 participants
When a higher-priority discovery client responds slowly,
ReactiveCompositeDiscoveryClientcurrently returns the faster lower-priority client's instances. This contradicts the ordering applied in its constructor and the behavior ofCompositeDiscoveryClient.Try the publishers in order with
switchIfEmpty, falling back only when a client completes without instances. The result stays streaming and preserves all instances from the first non-empty client. The sharedCloudFluxbehavior is unchanged.Fixes #1542.
Validation on JDK 21:
./mvnw -B -ntp -pl spring-cloud-commons -am test \ -Dtest=ReactiveCompositeDiscoveryClientTests,ReactiveCompositeDiscoveryClientAutoConfigurationTests,CompositeDiscoveryClientUnitTests \ -Dsurefire.failIfNoSpecifiedTests=false14 tests passed, with formatting and Checkstyle checks enabled. The delayed-priority regression failed on the original implementation. Coverage also includes empty fallback, multiple instances, error propagation, and not subscribing to lower-priority publishers once a result is found. The full repository suite was not run.
Developed with Codex assistance; the diff and regression results were reviewed.