Constructing a URL for execution

Question

I am working on a project in which I construct a URL with a valid hostname (but not a blocked hostname) and then execute that URL using RestTemplate from my main thread. I also have a single background thread in my application which parses the data from the URL and extracts the block list of hostnames from it.

If any block list of hostnames is present, then I won't make a call to that hostname from the main thread and I will try making a call to another hostname. By block list, I mean whenever any server is down, its hostname is on the block list.

Here is my background thread code. It will get the data from my service URL and keep on running every 10 minutes once my application has started up. It will then parse the data coming from the URL and store it in a ClientData class variable.

public class TempScheduler {

    // .. scheduledexecutors service code to start the background thread

    // call the service and get the data and then parse 
    // the response.
    private void callServiceURL() {
        String url = "url";
        RestTemplate restTemplate = new RestTemplate();
        String response = restTemplate.getForObject(url, String.class);
        parseResponse(response);

    }

    // parse the response and store it in a variable
    private void parseResponse(String response) {
        //...       

        // get the block list of hostnames
        Map<String, List<String>> coloExceptionList = gson.fromJson(response.split("blocklist=")[1], Map.class);
        List<String> blockList = new ArrayList<String>();
        for(Map.Entry<String, List<String>> entry : coloExceptionList.entrySet()) {
            for(String hosts : entry.getValue()) {
                blockList.add(hosts);
            }
        }

        // store the block list of hostnames which I am not supposed to make a call
        // from my main application
        ClientData.replaceBlockedHosts(blockList);
    }
}

Here is my ClientData class in which I am using CountDownLatch:

public class ClientData {

    // do I need this AtomicReference with ConcurrentHashMap?
    private static final AtomicReference<ConcurrentHashMap<String, String>> blockedHosts = new AtomicReference<>(
        new ConcurrentHashMap<String, String>());

    public static boolean isHostBlocked(String hostName) {
        return blockedHosts.get().containsKey(hostName);
    }

    public static void blockHost(String hostName) {
        blockedHosts.get().put(hostName, hostName);
    }

    public static void unblockHost(String hostName) {
        blockedHosts.get().remove(hostName);
    }

    public static void replaceBlockedHosts(List<String> hostNames) {
        ConcurrentHashMap<String, String> newBlockedHosts = new ConcurrentHashMap<>();
        for (String hostName : hostNames) {
            newBlockedHosts.put(hostName, hostName);
        }
        blockedHosts.set(newBlockedHosts);
    }
}

And here is my main application thread code in which I am finding all the hostnames on which I can make a call and then iterate the hostnames list to make a call.

If that hostname is null or in the block list category, then I won't make a call to that particular hostname and will try next hostname in the list.

@Override
public DataResponse call() throws Exception {

    List<String> hostnames = new LinkedList<String>();

    // .. some code here

    for (String hostname : hostnames) {     

        // If host name is null or host name is in block list category, skip sending request to this host
        if (hostname == null || ClientData.isHostBlocked(hostname)) {
            continue;
        }

        try {
            String url = generateURL(hostname);

            response = restTemplate.getForObject(url, String.class);

            break;
        } catch (RestClientException ex) {
            // log exception
            ClientData.blockHost(hostname);
        }
    }
}

I don't need to make a call to the hostname whenever it is down from the main thread. And my background thread gets these detail from one of my service, whenever any server is down, it will have the list of hostnames and whenever they are up, that list will get updated.

The above Callable code will be called at a rate of 1000 requests per second so it has to be fast. For the first time, whenever my blockListOfHosts is being updated from the background thread, I can return false instead of blocking the call using CountDownLatch but it has to be atomic, all the threads should see correct value of the block list of hostnames.

And also, whenever any RestClientException is being thrown, I will add that hostname in the blockListOfHosts as well since my background thread is running every 10 minutes, so that list won't have this hostname until 10 minutes is done. And whenever this server came back up, my background will update this list automatically.

Does my ClientData class looks right with the way I am using AtomicReference or not?

Answer 1

When I looked first I was thinking you don't need it cause ConcurrentHashMap has atomic operations.

But when I read your full ClientData I see that you create a map where the key is the same as your value.

Next thinking of me :

Isn't that a little overkill?

Why don't you just put a Set in your AtomicReference? The Set isn't atomic so then you shall need the AtomicReference again.

Then you could refactor to this :

private static final AtomicReference<Set<String>> blockedHosts = new AtomicReference<>(
    new HashSet<String>());

public static void replaceBlockedHosts(Set<String> hostNames) {
    blockedHosts.set(hostNames);
}

And change the building from hostNames to a set. You have the advantage with set that no duplicated entry's can be set.
The other 3 additional methods can be refactored as well with the use of a set.

Problem again :

Now that (not so) great idea I can put away. The set itself is NOT atomic and so there are questions about being thread safe.
So to the creator of the class, great idea to take the ConcurrentHashMap in stead of a Set.

So I'm starting all over.

Solution :

The AtomicReference is in mine opinion not necessary.
However, your setting and remove a entry of the Map is NOT thread safe. I should use the putIfAbsent(key, value) witch is an atomic operation.
The remove(Object key, Object value) is a atomic operation so I suggest you refactor the remove also to that method.

Answer 2

There are a few things to go through, and then a suggestion.

• You say you are using a CountDownLatch in your description, but you are not.
• You expect there to be null values in the dataset of hosts, that 'smells'.
• You take a 'defensive copy' of the hostnames so that you can loop over them, even though, in the normal case, you only need the first host in the list.
• you have implemented the ClientData as a class-of-static-methods. This is not 'idiomatic' for an OOP.

I think you need to work backwards from a 'this is the way I want it to work' concept, and then implement the ClientData class to match.

A sophisticated ClientData class would only give you valid hosts (or hosts it believes are valid). To do that, it is useful for the ClientData to know when a host succeeds, as well as when it fails. If it know when hosts are being used, it would also have the ability to load-balance different threads against different servers, as an added bonus.

This is how I would want to structure the use-case code:

    DataClients clients = ...... ; // some common reference system, or singleton.

    String hostname = null;
    while ((hostname = clients.nextAvailableHost(hostname)) != null) {
        try {
            String url = generateURL(hostname);

            return restTemplate.getForObject(url, String.class);

        } catch (RestClientException ex) {
            // log exception
            ClientData.blockHost(hostname);
        }
    }
    // log exception - no available/working servers.

The above code has the advantage that it can (using the nextAvailableHost method) simultaneously declare one host to be 'dead', while also requesting a replacement. The code is designed to take null-value for the dead host, or 'just get the next available host'.

Here's the way I would implement the ClientData class. Note that I use 'plain' synchronization here. There is no immediate need to use more complicated systems:

public static final class ClientData {
    private final LinkedList<String> validHosts = new LinkedList<>();

    public void replaceHosts(Collection<String> hosts) {
        synchronized (validHosts) {

            validHosts.clear();
            // remove nulls, make distinct.
            hosts.stream().filter(h -> h != null).distinct().forEach(h -> validHosts.add(h));
        }
    }

    public void failedHost(String host) {
        synchronized (validHosts) {
            validHosts.remove(host);
        }
    }

    public String nextAvailableHost(String failed) {
        synchronized(validHosts) {
            if (failed != null) {
                validHosts.remove(failed);
            }
            return requestHost();
        }
    }

    public String requestHost() {
        synchronized(validHosts) {
            // get a host, move to the end of the list.
            if (validHosts.isEmpty()) {
                return null;
            }
            String toret = validHosts.removeFirst();
            validHosts.addLast(toret);
            return toret;
        }
    }

}

Then, your background process is relatively simple. Every 10 minutes it just calls:

    Set<String> usehosts = new HashSet<>();
    usehosts.addAll( ..... ); // the normal list of hosts.

    // get the block list of hostnames
    Map<String, List<String>> coloExceptionList = gson.fromJson(response.split("blocklist=")[1], Map.class);
    List<String> blockList = new ArrayList<String>();
    for(Map.Entry<String, List<String>> entry : coloExceptionList.entrySet()) {
        for(String hosts : entry.getValues()) {
            usehosts.remove(hosts);
        }
    }    

    clientData.replaceHosts(usehosts);