Why the bean of 'SameInstancePreferenceServiceInstanceListSupplier' use 'withCaching()' method

[ReionChan]

The bean of SameInstancePreferenceServiceInstanceListSupplier in the LoadBalancerClientConfiguration:

@Bean
@ConditionalOnBean(DiscoveryClient.class)
@ConditionalOnMissingBean
@Conditional(SameInstancePreferenceConfigurationCondition.class)
public ServiceInstanceListSupplier sameInstancePreferenceServiceInstanceListSupplier(
        ConfigurableApplicationContext context) {

         // *** Config to use cache by calling method withCaching(), Why? ***
    return ServiceInstanceListSupplier.builder().withBlockingDiscoveryClient().withSameInstancePreference()
            .withCaching().build(context);
}

When the SameInstancePreferenceServiceInstanceListSupplier is delegated to CachingServiceInstanceListSupplier, the method 'selectedServiceInstance(ServiceInstance)' which is defined in SelectedInstanceCallback and implemented by SameInstancePreferenceServiceInstanceListSupplier has no chance to be call in the RoundRobinLoadBalancer or RandomLoadBalancer:

public Mono<Response<ServiceInstance>> choose(Request request) {
    ServiceInstanceListSupplier supplier = serviceInstanceListSupplierProvider
        .getIfAvailable(NoopServiceInstanceListSupplier::new);
    return supplier.get(request).next()
        .map(serviceInstances -> processInstanceResponse(supplier, serviceInstances));
}

private Response<ServiceInstance> processInstanceResponse(ServiceInstanceListSupplier supplier,
    List<ServiceInstance> serviceInstances) {
    Response<ServiceInstance> serviceInstanceResponse = getInstanceResponse(serviceInstances);

    // *** The type of supplier is CachingServiceInstanceListSupplier so it will skip this if statement ***
    if (supplier instanceof SelectedInstanceCallback && serviceInstanceResponse.hasServer()) {
        ((SelectedInstanceCallback) supplier).selectedServiceInstance(serviceInstanceResponse.getServer());
    }
    return serviceInstanceResponse;
}

This will result in the field named previouslyReturnedInstance in SameInstancePreferenceServiceInstanceListSupplier not being assigned a value, then the feature of SAME INSTANCE PREFERENCE would not working.

So is it necessary to add withCaching() in the LoadBalancerClientConfiguration to define the 'SameInstancePreferenceServiceInstanceListSupplier' bean?

[OlgaMaciaszek]

Hello @ReionChan, thanks for raising the issue. I believe this fix solves the problem.

[ReionChan]

Hi @OlgaMaciaszek, thanks for solving this problem, it works very well.

But I found a new problem:

When a CachingServiceInstanceListSupplier delegates a class which implements the method ServiceInstanceListSupplier#get(Request request), the CachingServiceInstanceListSupplier will always call the NO ARGS method of the delegate Supplier#get(). This will result in the logic code defined in get(Request request) not be executed, then losing the original delegate's feature.

Code in CachingServiceInstanceListSupplier

public CachingServiceInstanceListSupplier(ServiceInstanceListSupplier delegate, CacheManager cacheManager) {
    super(delegate);
    this.serviceInstances = CacheFlux.lookup(key -> {

    }, delegate.getServiceId()).onCacheMissResume(
        //-----------------------------------------------
        //  calling delegate NO ARGS method
        //-----------------------------------------------
        delegate.get().take(1))
        .andWriteWith((key, signals) -> Flux.fromIterable(signals).dematerialize().doOnNext(instances -> {
            Cache cache = cacheManager.getCache(SERVICE_INSTANCE_CACHE_NAME);
            if (cache == null) {
                if (log.isErrorEnabled()) {
                    log.error("Unable to find cache for writing: " + SERVICE_INSTANCE_CACHE_NAME);
                }
            }
            else {
                cache.put(key, instances);
            }
        }).then());
}

Take the RequestBasedStickySessionServiceInstanceListSupplier for example: RequestBasedStickySessionServiceInstanceListSupplier bean defined in LoadBalancerClientConfiguration :

@Bean
@ConditionalOnBean(ReactiveDiscoveryClient.class)
@ConditionalOnMissingBean
@Conditional(RequestBasedStickySessionConfigurationCondition.class)
public ServiceInstanceListSupplier requestBasedStickySessionDiscoveryClientServiceInstanceListSupplier(
        ConfigurableApplicationContext context) {

    // delegated by CachingServiceInstanceListSupplier using withCaching()
    return ServiceInstanceListSupplier.builder().withDiscoveryClient().withRequestBasedStickySession()
            .withCaching().build(context);
}

Because of wrapping in CachingServiceInstanceListSupplier, the sticky session feature not working.

Expecting a reply, thanks a lot!

[OlgaMaciaszek]

Hi @ReionChan, that's right. We already have an issue for this. Closing in favour of gh-1221.

[OlgaMaciaszek]

Hello, @ReionChan, on a closer look, I think you are right; since it would not make sense to cache per requests, the CachingServiceInstanceListSupplier and the HealthCheckServiceInstanceListSupplier that provide the caching mechanism, both should go in the hierarchy directly after the supplier that retrieves instances over the network, before any further filtering takes place. I will fix it now.

[ReionChan]

Hello, @OlgaMaciaszek, Yes, the main purpose of caching is to reduce network overhead, not to cache subsequent request-based or filtering functions together, so there is no need to force CachingServiceInstanceListSupplier to be at the top of the delegate. Compared with before modification, the current cache layer is more in line with the original purpose. Cheers 🎉