Simple load balancer

Question

I would like to know the possible improvements in design and concurrency of the following load balancer:

1. ability to add/register new instance
2. keep max of 10 instances for load balancer
3. forbid duplicates for available instances
4. on add operation check for if instances are alive, per random
5. ability to return random instance

Questions:

1. Is is better to return boolean value indicating that instance registration was successful, or to return void and throw checked/unchecked exception indicating it went wrong?
2. How could this code be improved from concurrency perspective?

import lombok.Getter;

import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.random.RandomGenerator;

import static java.util.stream.Collectors.toCollection;

public final class SimpleLoadBalancerV1 {

    private final ReadWriteLock lock;
    private final InstanceHealthChecker healthChecker;
    private final InstanceSelector selector;
    private final int maxInstances;
    private List<Instance> instances;

    public SimpleLoadBalancerV1(int maxInstances, InstanceSelector selector, InstanceHealthChecker healthChecker) {
        if (maxInstances <= 0) throw new IllegalArgumentException("invalid maxInstances");
        this.maxInstances = maxInstances;
        this.selector = selector;
        this.healthChecker = healthChecker;
        this.lock = new ReentrantReadWriteLock();
        this.instances = new ArrayList<>();
    }

    public boolean registerInstance(Instance newInstance) {
        lock.writeLock().lock();
        try {
            if (instances.size() == maxInstances) return false;
            if (instances.contains(newInstance)) return false;

            var newInstances = detectAliveInstances(instances);
            newInstances.add(newInstance);

            instances = newInstances;
        } finally {
            lock.writeLock().unlock();
        }
        return true;
    }

    public Instance getInstance() {
        lock.readLock().lock();
        try {
            return selector.select(instances);
        } finally {
            lock.readLock().unlock();
        }
    }

    private List<Instance> detectAliveInstances(List<Instance> instances) {
        return instances.stream().filter(healthChecker::isAlive).collect(toCollection(ArrayList::new));
    }

    public interface InstanceHealthChecker {
        boolean isAlive(Instance instance);
    }
    
    public static final class RandomHealthChecker implements InstanceHealthChecker {

        private final RandomGenerator generator;

        public RandomHealthChecker(RandomGenerator generator) {
            this.generator = generator;
        }

        @Override
        public boolean isAlive(Instance instance) {
            return generator.nextBoolean();
        }
    }

    public interface InstanceSelector {
        Instance select(List<Instance> instances);
    }

    public static final class RandomInstanceSelector implements InstanceSelector {

        private final RandomGenerator generator;

        public RandomInstanceSelector(RandomGenerator generator) {
            this.generator = generator;
        }

        @Override
        public Instance select(List<Instance> instances) {
            return instances.get(generator.nextInt(instances.size()));
        }
    }

    @Getter
    public static final class Instance {

        private final String name;

        public Instance(String name) {
            this.name = name;
        }

        @Override
        public boolean equals(Object o) {
            if (this == o) return true;
            if (o == null || getClass() != o.getClass()) return false;
            Instance instance = (Instance) o;
            return Objects.equals(name, instance.name);
        }

        @Override
        public int hashCode() {
            return Objects.hash(name);
        }
    }
}

Answer 1

Returning boolean vs throwing checked/unchecked Exception

Throwing an Exception offers more flexibility because it creates a possibility for leveraging centralized error-handling (for instance using ControllerAdvice in a Spring application), rather than handling a failure right on the spot.

On the other hand, when a method boolean, the caller needs to check the return value immediately to react to a failure.

If you choose to throw an exception, avoid forcing users of your API dealing with checked exceptions.

Inconsistent behavior while adding a new Instance

You're purging Instances that not alive while adding a new Instance, but only if the maximum size is not reached and the candidate instance isn't a duplicate:

try {
    if (instances.size() == maxInstances) return false;
    if (instances.contains(newInstance)) return false;
        
    var newInstances = detectAliveInstances(instances);
    newInstances.add(newInstance);
        
    instances = newInstances;
}

Consider the case when the limit reached, but a significant portion of instances are down. Why disallow adding a new instance in this case?

^{Yes, in this dummy implementation health-check simulated using a random generator, but it doesn't invalidate the point}

Also, why not to check if the candidate instance is alive?

Picking a random instance

Are we ok with returning from getInstance() an instance which is currently down? Maybe we should perform a health-check?

Nesting

Why nesting Instance class inside the load balancer? It would make sense only if the usage of Instance is confined to load balancer internal mechanics.

But what you have is the opposite, the Instances a load balancer operates with originate outside of it.

Same for Selector and InstanceHealthChecker.

Naming

SimpleLoadBalancerV1 - a name is not a suitable place to place information about the version of the code element, beside that v1, v2 isn't telling a lot. Avoid doing that.

Instead, you can use commit comments in your Git repository to give a more comprehensive information about changes in a code element.

The name of a code element should only communicate its purpose.

Instance.equals()

instanceof encompasses both null-check and whether an object is an instance of a particular type. And in the case it will be an equivalent of a class-check, since you declared Instance class a final, it can not be extended.

So

@Override
public boolean equals(Object o) {
    if (this == o) return true;
    if (o == null || getClass() != o.getClass()) return false;
    Instance instance = (Instance) o;
    return Objects.equals(name, instance.name);
}

Can be written as

@Override
public boolean equals(Object o) {
    if (this == o) return true;
    return o instanceof Instance other
        && Objects.equals(name, other.name);
}

Answer 2

Concerning your first question, your error response is effectively "something went wrong". I prefer the "no news is good news" approach where successful operation returns nothing and errors are reported with exceptions. You have these two different error cases that should have a different error message.

if (instances.size() == maxInstances) return false;
if (instances.contains(newInstance)) return false;

Based on the fact that you create a new list when adding an instance, it seems like your intention is that the content of the list of instances never changes after it has been created. Using Collections.unmodifiableList() to ensure this would convey the intention without doubt. Then you only need to add a comment explaining why you want to do this.

One-line ifs like the one below are pretty hard to follow as they make the line length excessively long. The message should say "illegal" as the exception is named "illegal argument". But that's a pretty minor thing.

if (maxInstances <= 0) throw new IllegalArgumentException("invalid maxInstances");

One-line streams are pretty hard to follow. You seem to have a bit of a desire to reduce the line count by putting a lot of stuff on the same line. I don't think this is a good practise. I prefer having each stream operation on a separate line. (And once you get a version control running, it helps you limit changes to relevant lines.)

return instances.stream()
    .filter(healthChecker::isAlive)
    .collect(toCollection(ArrayList::new));

I think using Lombok to generate getters but not equals and hashCode is the wrong way. In my experience you almost never need to access the equals and hashCode methods during debugging but the frustration of not being able to put a breakpoint on a constructor, setter or getter (because there is no code for it), is somewhat common. Lombok is best when used to cover noisy boilerplate like logging and equals methods. For everything else I think it's counterproductive.