Enhance RoundRoubinLoadBalancer position

[NeatGuyCoding]

Is your feature request related to a problem? Please describe.

Assume there are retries for a rpc call to a remote service, and the retry scheme is retry twice for a call. Assume there are two instances (instance A and instance B) for this service. We want the retry is made twice and each time to a different instance.

But there is a problem while using the RoundRobinLoadBalancer. The position is a Atomic field shared across threads and there would be a case that the retry is made to the same instance to the last time:

• Thread 1: position = 0, call A;
• Thread 2: position = 1, call B;
• Thread 1: call A failed, position = 2, still retry A;

Describe the solution you'd like

Make the position a thread local field and each initialize with random value:

ThreadLocal<Long> position =  ThreadLocal.withInitial(() ->  ThreadLocalRandom.current().nextInt(1000));

[NeatGuyCoding]

Well, I found that the result is retrived through:

supplier.get().next().map(this::getInstanceResponse);

this is assined to another thread in reactor, therefore the threadlocal position is not good enough.

Consider usinf context to keep origin call context

[NeatGuyCoding]

one solution is that:

private final ConcurrentHashMap<Long, Integer> position;

 public Mono<Response<ServiceInstance>> choose(Request request) {
        // TODO: move supplier to Request?
        // Temporary conditional logic till deprecated members are removed.
        long threadId = Thread.currentThread().getId();
        if (serviceInstanceListSupplierProvider != null) {
            ServiceInstanceListSupplier supplier = serviceInstanceListSupplierProvider
                    .getIfAvailable(NoopServiceInstanceListSupplier::new);
            return supplier.get().next().map(serviceInstances -> getInstanceResponse(serviceInstances, threadId));
        }
        ServiceInstanceSupplier supplier = this.serviceInstanceSupplier
                .getIfAvailable(NoopServiceInstanceSupplier::new);
        return supplier.get().collectList().map(serviceInstances -> getInstanceResponse(serviceInstances, threadId));
    }

    private Response<ServiceInstance> getInstanceResponse(
            List<ServiceInstance> instances, long threadId) {
        if (instances.isEmpty()) {
            log.warn("No servers available for service: " + this.serviceId);
            return new EmptyResponse();
        }
        // TODO: enforce order?
        int pos = Math.abs(this.position.computeIfAbsent(threadId, k -> ThreadLocalRandom.current().nextInt(1000)));
        this.position.put(threadId, pos + 1);
        ServiceInstance instance = instances.get(pos % instances.size());
        return new DefaultResponse(instance);
    }

[spencergibb]

A few issues. The number of instances is low these algorithms work much better with higher numbers of instances. Can you increase the retry count?

[NeatGuyCoding]

@spencergibb increase the retry count will cause the service enter circuit breaker on more often and increase the possibility of avalanche which is not preferred. Moreover, the more the number of threads, the more possibility of retrying same instance

[NeatGuyCoding]

A further solution is to create a random position for each request Id (when including sleuth, it should be traceId). Ensure the position is separated by request Id and retry will not be executed on same instance before

[OlgaMaciaszek]

@HashZhang That's a good point. We are now working on adding retry support and we'll probably be adding a similar solution there.

[OlgaMaciaszek]

Closing in favour of #659