Calling other machines from the LinkedList if one is blocked or down

Question

This is a follow on to : Simplifying asynchronous "executeAsync" method along with "onFailure" callback.

I am using AsyncRestTemplate as my HttpClient to execute my URL and the server will return back a json string as the response. Customer will call this library by passing DataKey object which has userId in it.

• Given a userId, I will find out what are the machines that I can hit to get the data and then store those machines in a LinkedList so that I can execute them sequentially.
• After that I will check whether the first hostname is in block list or not. If it is not there in the block list, then I will make a URL with the first hostname in the list and execute it and if the response is successful then return back the response. But let's say if that first hostname is in the block list, then I will try to get the second hostname in the list and make the url and execute it so basically find the hostname which is not in block list before making the URL.
• Now, let's say if we selected first hostname which was not in the block list and executed the URL and somehow server was down or not responding, then I will execute the second hostname in the list and keep doing this until you get a successful response. But make sure they were not in the block list as well so we need to follow above point.
• If all servers are down or in block list, then I can simply log and return the error that service is unavailable.

Below is my DataClient class which will be called by customer and they will pass DataKey object to getData method.

public class DataClient implements Client, DataFetcher {

    private final AsyncRestTemplate restTemplate = new AsyncRestTemplate(new HttpComponentsAsyncClientHttpRequestFactory());

    @Override
    public ListenableFuture<DataResponse> getData(DataKey key) {
        final SettableFuture<DataResponse> responseFuture = SettableFuture.create();

        // given a userId, find all the hostnames 
        // so it can also have four hostname or one hostname or six hostname as well in the list
        LinkedList<String> listOfHostnames = getHostnamesInfo(key.getUserId());

        // any way to simplify this?
        boolean isFound = false;
        while (!listOfHostnames .isEmpty()) {
            String firstHostname = listOfHostnames .removeFirst();
            if (!ClientUtils.isEmpty(firstHostname) && !ShardMapping.isBlockHost(firstHostname)) {
                executeForServers(responseFuture, key, firstHostname, listOfHostnames );
                isFound = true;
                break;
            }
        }
        // all the servers were in block list
        if (!isFound) {
            responseFuture.set(new DataResponse(null, DataErrorEnum.SERVICE_UNAVAILABLE,
                    DataStatusEnum.ERROR));
        }

        return responseFuture;
    }

    @Override
    public void executeForServers(SettableFuture<DataResponse> responseFuture, DataKey key, String hostname,
            LinkedList<String> restHostnames) {

        // should I add the logic here?

        ListenableFutureCallback<ResponseEntity<String>> callback = new RetryCallback(this, responseFuture,
                hostname, restHostnames, key);
        restTemplate.exchange(generateURL(hostname, key), HttpMethod.GET, key.getEntity(),
                String.class).addCallback(callback);
    }
}

Below is my interface for DataFetcher:

public interface DataFetcher {
    void executeForServers(SettableFuture<DataResponse> responseFuture, DataKey key, String hostname,
            LinkedList<String> restHostnames);
}

Below is my RetryCallback class:

public class RetryCallback implements ListenableFutureCallback<ResponseEntity<String>> {
    private final SettableFuture<DataResponse> responseFuture;
    private final LinkedList<String> restHostnames;
    private final String hostname;
    private final DataKey key;
    private final DataFetcher dataFetcher;

    private RetryCallback(final DataFetcher dataFetcher, final SettableFuture<DataResponse> responseFuture,
            final String hostname, final LinkedList<String> restHostnames, final DataKey key) {
        this.dataFetcher = checkNotNull(dataFetcher);
        this.responseFuture = checkNotNull(responseFuture);
        this.hostname = checkNotNull(hostname);
        this.restHostnames = checkNotNull(restHostnames);
        this.key = checkNotNull(key);
    }

    @Override
    public void onSuccess(ResponseEntity<String> result) {
         responseFuture.set(new DataResponse(result.getBody(), DataErrorEnum.OK,
                DataStatusEnum.SUCCESS));
    }

    // any way to simplify this?
    // recursively calling executeForServers method
    @Override
    public void onFailure(final Throwable ex) {
        if (ex instanceof SocketException) {
            // if it comes here, then it means some of the servers are down so adding it into block list
            ShardMapping.blockHost(hostname);
            while (!restHostnames.isEmpty()) {
                String nextHostname = restHostnames.removeFirst();
                if (!ClientUtils.isEmpty(nextHostname) && !ShardMapping.isBlockHost(nextHostname)) {  // check to see whether hostname is blocked or not
                    dataFetcher.executeForServers(responseFuture, key, nextHostname, restHostnames);
                    return;
                }
            }               
            // either  all the servers are down or all the servers were in block list
            if (ClientUtils.isEmpty(restHostnames)) {
                responseFuture.set(new DataResponse(null, DataErrorEnum.SERVICE_UNAVAILABLE,
                        DataStatusEnum.ERROR));
            }
        } else { // this is for 4xx (HttpClientErrorException) and 5xx (HttpServerErrorException) error coming from server side
            HttpStatusCodeException httpException = (HttpStatusCodeException) ex;
            DataErrorEnum error = DataErrorEnum.getErrorEnumByException(httpException);
            responseFuture.set(new DataResponse(httpException.getResponseBodyAsString(), error,
                    DataStatusEnum.ERROR));
        }
        // is there any other error I should look into?
    }
}

My block list keeps getting updated from another background thread every 1 minute. If any server is down and not responding, then I need to block that server by using this -

ShardMapping.blockHost(hostname);

And to check whether any server is in block list or not, I use this -

ShardMapping.isBlockHost(hostname);

I have my above code working fine so I am opting for code review to see whether we can simplify my above four points or not.

• As of now, I am doing same check at two places, one in getData() method and other in onFailure() method as well so wanted to see whether is there any room for improvement or not.
• Also, is there any other error should I handle in my onFailure command? As of now, it only checks for HttpClientErrorException, HttpServerErrorException and SocketException.

Answer 1

1. Name of getHostnamesInfo is misleading a little bit. It returns to listOfHostnames, so calling it getHostnames or getHostnamesForUser would be less confusing.

2. First, to DRY the code, I'd try to extract out some hostname list handling logic to a separate class:

   public class Hosts {
   
       private final LinkedList<String> hostsnames = newLinkedList();
   
       public Hosts(final List<String> hosts) {
           checkNotNull(hosts, "hosts cannot be null");
           this.hostsnames.addAll(hosts);
       }
   
       public Optional<String> getNextAvailableHostname() {
           while (!hostsnames.isEmpty()) {
               String firstHostname = hostsnames.removeFirst();
               if (!ClientUtils.isEmpty(firstHostname) && !ShardMapping.isBlockHost(firstHostname)) {
                   return Optional.of(firstHostname);
               }
           }
           return Optional.absent();
       }
   
       public boolean isEmpty() {
           return hostsnames.isEmpty();
       }
   }
   

   It makes getData simpler:

   @Override
   public ListenableFuture<DataResponse> getData(final DataKey key) {
       // given a userId, find all the hostnames
       // so it can also have four hostname or one hostname or six hostname as
       // well in the list
       final LinkedList<String> hostnames = getHostnames(key.getUserId());
       final Hosts hosts = new Hosts(hostnames);
       final Optional<String> nextAvailableHost = hosts.getNextAvailableHostname();
       if (!nextAvailableHost.isPresent()) {
           final SettableFuture<DataResponse> responseFuture = SettableFuture.create();
           responseFuture.set(new DataResponse(null, DataErrorEnum.SERVICE_UNAVAILABLE, DataStatusEnum.ERROR));
           return responseFuture;
       }
       checkState(nextAvailableHost.isPresent());
   
       final SettableFuture<DataResponse> responseFuture = SettableFuture.create();
       executeForServers(responseFuture, key, nextAvailableHost.get(), hosts);
   
       return responseFuture;
   }
   

   and improves a little bit the RetryCallback.onFailure too:

   @Override
   public void onFailure(final Throwable ex) {
       if (ex instanceof SocketException) {
           // if it comes here, then it means some of the servers are down so adding it into block list
           ShardMapping.blockHost(hostname);
   
           final Optional<String> nextAvailableHost = hosts.getNextAvailableHostname();
           if (nextAvailableHost.isPresent()) {
               dataFetcher.executeForServers(responseFuture, key, nextAvailableHost.get(), hosts);
               return;
           }
   
           // either  all the servers are down or all the servers were in block list
           if (hosts.isEmpty()) {
               responseFuture.set(new DataResponse(null, DataErrorEnum.SERVICE_UNAVAILABLE, DataStatusEnum.ERROR));
           }
       } else { // this is for 4xx (HttpClientErrorException) and 5xx (HttpServerErrorException) error coming from server side
           HttpStatusCodeException httpException = (HttpStatusCodeException) ex;
           DataErrorEnum error = DataErrorEnum.getErrorEnumByException(httpException);
           responseFuture.set(new DataResponse(httpException.getResponseBodyAsString(), error,
                   DataStatusEnum.ERROR));
       }
       // is there any other error I should look into?
   }
   

3. You might notice some other duplication here, especially if you invert the condition in getData:

   final SettableFuture<DataResponse> responseFuture = SettableFuture.create();
   
   final Optional<String> nextAvailableHost = hosts.getNextAvailableHostname();
   if (nextAvailableHost.isPresent()) {
       executeForServers(responseFuture, key, nextAvailableHost.get(), hosts);
   } else {
       responseFuture.set(new DataResponse(null, DataErrorEnum.SERVICE_UNAVAILABLE, DataStatusEnum.ERROR));
   }
   
   return responseFuture;
   

   I have created a temporary execute method for it in the Hosts class:

   public void execute(final SettableFuture<DataResponse> responseFuture, final DataKey key, final DataFetcher dataFetcher) {
       final Optional<String> nextAvailableHost = getNextAvailableHostname();
       if (nextAvailableHost.isPresent()) {
           dataFetcher.executeForServers(responseFuture, key, nextAvailableHost.get(), this);
           return;
       }
   
       responseFuture.set(new DataResponse(null, DataErrorEnum.SERVICE_UNAVAILABLE, DataStatusEnum.ERROR));
   }
   

   It probably has a bad name and probably it should not be in the Hosts class but it's enough in this internal step.

   Now getData looks like this:

   @Override
   public ListenableFuture<DataResponse> getData(final DataKey key) {
       final LinkedList<String> hostnames = getHostnames(key.getUserId());
       final Hosts hosts = new Hosts(hostnames);
       final SettableFuture<DataResponse> responseFuture = SettableFuture.create();
       hosts.execute(responseFuture, key, this);
       return responseFuture;
   }
   

   And onFailure:

   @Override
   public void onFailure(final Throwable ex) {
       if (ex instanceof SocketException) {
           // if it comes here, then it means some of the servers are down so adding it into block list
           ShardMapping.blockHost(hostname);
           hosts.execute(responseFuture, key, dataFetcher);
       } else { // this is for 4xx (HttpClientErrorException) and 5xx (HttpServerErrorException) error coming from server side
           HttpStatusCodeException httpException = (HttpStatusCodeException) ex;
           DataErrorEnum error = DataErrorEnum.getErrorEnumByException(httpException);
           responseFuture.set(new DataResponse(httpException.getResponseBodyAsString(), error,
                   DataStatusEnum.ERROR));
       }
   }
   

4. I don't feel that Hosts class fulfills the single responsibility principle. It handles hostnames well but the execute method seems another responsibility. Furthermore, it uses only the public API of Hosts which does not suggest high cohesion. So I moved this method to a new class:

   public class Retryer {
   
       public void execute(final SettableFuture<DataResponse> responseFuture, final DataKey key,
               final DataFetcher dataFetcher, final Hosts hosts) {
           final Optional<String> nextAvailableHost = hosts.getNextAvailableHostname();
           if (nextAvailableHost.isPresent()) {
               dataFetcher.executeForServers(responseFuture, key, nextAvailableHost.get(), hosts);
               return;
           }
   
           responseFuture.set(new DataResponse(null, DataErrorEnum.SERVICE_UNAVAILABLE, DataStatusEnum.ERROR));
       }
   }
   

   Usage in onFailure:

   new Retryer().execute(responseFuture, key, dataFetcher, hosts);
   

   and in getData:

   new Retryer().execute(responseFuture, key, this, hosts);
   

   Note that isEmpty in Hosts is not used, you could remove it. Furthermore, the Retryer name does not looks good, it smells for me.

5. Another duplication is in this line:

   responseFuture.set(new DataResponse(httpException.getResponseBodyAsString(), error,
               DataStatusEnum.ERROR));
   

   and this one:

   responseFuture.set(new DataResponse(null, DataErrorEnum.SERVICE_UNAVAILABLE, DataStatusEnum.ERROR));
   

   I moved these calls also to the Retryer class:

   public void execute(final SettableFuture<DataResponse> responseFuture, final DataKey key,
           final DataFetcher dataFetcher, final Hosts hosts) {
       final Optional<String> nextAvailableHost = hosts.getNextAvailableHostname();
       if (nextAvailableHost.isPresent()) {
           dataFetcher.executeForServers(responseFuture, key, nextAvailableHost.get(), hosts);
           return;
       }
   
       setServiceUnavailable(responseFuture);
   }
   
   private void setServiceUnavailable(final SettableFuture<DataResponse> responseFuture) {
       final String errorMessage = null;
       final DataErrorEnum error = DataErrorEnum.SERVICE_UNAVAILABLE;
       setErrorReponse(responseFuture, errorMessage, error);
   }
   
   public void setErrorReponse(final SettableFuture<DataResponse> responseFuture, final String errorMessage,
           final DataErrorEnum error) {
       final DataResponse dataResponse = new DataResponse(errorMessage, error, DataStatusEnum.ERROR);
       responseFuture.set(dataResponse);
   }
   

6. I would consider creating a new constructor in the DataResponse which omits the errorMessage parameter so passing null does not clutter the code. (When I see a null parameter I always wonder which parameter is it. Unfortunately I have to check the implementation to figure it out.)

7. Another thing is that errorMessage can be null. Setting it to an empty string might save you from a few NullPointerExceptions later (if that's possible and sane).

8. Let's go back to the ugly Retryer. Currently it's the only one caller of executeForServers. Additionally, I have a feeling that I would prefer to call it DataFetcherExecutor or DataFetcher which is even better. Now, I changed DataFetcher from interface to class and moved both Retryer.execute and DataClient.executeForServers into it (along with their dependencies). When I've finished I got this:

   public class DataFetcher {
   
       private final AsyncRestTemplate restTemplate = new AsyncRestTemplate(
               new HttpComponentsAsyncClientHttpRequestFactory());
   
       public void execute(final SettableFuture<DataResponse> responseFuture, final DataKey key, final Hosts hosts) {
           final Optional<String> nextAvailableHost = hosts.getNextAvailableHostname();
           if (nextAvailableHost.isPresent()) {
               executeForServers(responseFuture, key, nextAvailableHost.get(), hosts);
               return;
           }
   
           setServiceUnavailable(responseFuture);
       }
   
       public void executeForServers(final SettableFuture<DataResponse> responseFuture, final DataKey key,
               final String hostname, final Hosts hosts) {
           final ListenableFutureCallback<ResponseEntity<String>> callback = new RetryCallback(this, responseFuture,
                   hostname, key, hosts);
           restTemplate.exchange(generateURL(hostname, key), HttpMethod.GET, key.getEntity(), String.class).addCallback(
                   callback);
       }
   
       private void setServiceUnavailable(final SettableFuture<DataResponse> responseFuture) {
           final String errorMessage = null;
           final DataErrorEnum error = DataErrorEnum.SERVICE_UNAVAILABLE;
           setErrorReponse(responseFuture, errorMessage, error);
       }
   
       public void setErrorReponse(final SettableFuture<DataResponse> responseFuture, final String errorMessage,
               final DataErrorEnum error) {
           final DataResponse dataResponse = new DataResponse(errorMessage, error, DataStatusEnum.ERROR);
           responseFuture.set(dataResponse);
       }
   
       ... // generateURL is here
   
   }
   

   Retryer is empty, it can be removed now. DataClient also a lot simpler, it does not implement DataFetcher any more and it contains only some simple wiring logic:

   public class DataClient implements Client {
   
       @Override
       public ListenableFuture<DataResponse> getData(final DataKey key) {
           final LinkedList<String> hostnames = getHostnames(key.getUserId());
           final Hosts hosts = new Hosts(hostnames);
           final SettableFuture<DataResponse> responseFuture = SettableFuture.create();
           final DataFetcher dataFetcher = new DataFetcher();
           dataFetcher.execute(responseFuture, key, hosts);
           return responseFuture;
       }
   
       ... // getHostnames is here
   }
   

   And RetryCallback:

   public class RetryCallback implements ListenableFutureCallback<ResponseEntity<String>> {
       private final SettableFuture<DataResponse> responseFuture;
       private final String hostname;
       private final DataKey key;
       private final DataFetcher dataFetcher;
       private final Hosts hosts;
   
       public RetryCallback(final DataFetcher dataFetcher, final SettableFuture<DataResponse> responseFuture,
               final String hostname, final DataKey key, final Hosts hosts) {
           this.dataFetcher = checkNotNull(dataFetcher);
           this.responseFuture = checkNotNull(responseFuture);
           this.hostname = checkNotNull(hostname);
           this.key = checkNotNull(key);
           this.hosts = checkNotNull(hosts);
       }
   
       @Override
       public void onSuccess(ResponseEntity<String> result) {
            responseFuture.set(new DataResponse(result.getBody(), DataErrorEnum.OK,
                   DataStatusEnum.SUCCESS));
       }
   
       @Override
       public void onFailure(final Throwable ex) {
           if (ex instanceof SocketException) {
               // if it comes here, then it means some of the servers are down so adding it into block list
               ShardMapping.blockHost(hostname);
               dataFetcher.execute(responseFuture, key, hosts);
           } else { // this is for 4xx (HttpClientErrorException) and 5xx (HttpServerErrorException) error coming from server side
               HttpStatusCodeException httpException = (HttpStatusCodeException) ex;
               DataErrorEnum error = DataErrorEnum.getErrorEnumByException(httpException);
               final String errorMessage = httpException.getResponseBodyAsString();
               dataFetcher.setErrorReponse(responseFuture, errorMessage, error);
           }
       }
   }
   

   For me it seems quite good and simple, but do not stop yet.

9. Let's try to move body of onSuccess to DataFetcher as well. DataFetches contains some similar code already.

   public void setSuccessResponse(final SettableFuture<DataResponse> responseFuture, final String message) {
       final DataResponse dataResponse = new DataResponse(message, DataErrorEnum.OK, DataStatusEnum.SUCCESS);
       responseFuture.set(dataResponse);
   }
   

   Usage in RetryCallback:

   @Override
   public void onSuccess(ResponseEntity<String> result) {
       final String message = result.getBody();
       dataFetcher.setSuccessResponse(responseFuture, message);
   }
   

10. Notice in RetryCallback that dataFetcher is the only one object which uses the responseFuture reference of RetryCallback. It might be in the DataFetcher class and probably RetryCallback don't have to know anything about responseFuture, it should be an internal detail of DataFetcher. Let's try that and the same with the hosts and key fields too.

    DataFetcher with the new fields and constructor parameters:

    public class DataFetcher {
    
        private final AsyncRestTemplate restTemplate = new AsyncRestTemplate(
                new HttpComponentsAsyncClientHttpRequestFactory());
    
        private final SettableFuture<DataResponse> responseFuture;
        private final DataKey key;
        private final Hosts hosts;
    
        public DataFetcher(final SettableFuture<DataResponse> responseFuture, final DataKey key, final Hosts hosts) {
            this.responseFuture = checkNotNull(responseFuture);
            this.key = checkNotNull(key);
            this.hosts = checkNotNull(hosts);
        }
    
        public void tryNextHost() {
            final Optional<String> nextAvailableHost = hosts.getNextAvailableHostname();
            if (nextAvailableHost.isPresent()) {
                doExchange(nextAvailableHost.get());
                return;
            }
    
            setServiceUnavailable();
        }
    
        private void doExchange(final String hostname) {
            final ListenableFutureCallback<ResponseEntity<String>> callback = new RetryCallback(this, hostname);
            final URI url = generateURL(hostname, key);
            restTemplate.exchange(url, HttpMethod.GET, key.getEntity(), String.class).addCallback(
                    callback);
        }
    
        private void setServiceUnavailable() {
            final String errorMessage = null;
            final DataErrorEnum error = DataErrorEnum.SERVICE_UNAVAILABLE;
            setErrorReponse(errorMessage, error);
        }
    
        public void setErrorReponse(final String errorMessage, final DataErrorEnum error) {
            final DataResponse dataResponse = new DataResponse(errorMessage, error, DataStatusEnum.ERROR);
            responseFuture.set(dataResponse);
        }
    
        public void setSuccessResponse(final String message) {
            final DataResponse dataResponse = new DataResponse(message, DataErrorEnum.OK, DataStatusEnum.SUCCESS);
            responseFuture.set(dataResponse);
        }
    
        ...
    }
    

    Note the removed responseFuture parameters of almost every method. I've also renamed a few methods.

    Now, here is RetryCallback:

    public class RetryCallback implements ListenableFutureCallback<ResponseEntity<String>> {
        private final String hostname;
        private final DataFetcher dataFetcher;
    
        public RetryCallback(final DataFetcher dataFetcher, final String hostname) {
            this.dataFetcher = checkNotNull(dataFetcher);
            this.hostname = checkNotNull(hostname);
        }
    
        @Override
        public void onSuccess(final ResponseEntity<String> result) {
            final String message = result.getBody();
            dataFetcher.setSuccessResponse(message);
        }
    
        @Override
        public void onFailure(final Throwable ex) {
            if (ex instanceof SocketException) {
                // if it comes here, then it means some of the servers are down so
                // adding it into block list
                ShardMapping.blockHost(hostname);
                dataFetcher.tryNextHost();
            } else { // this is for 4xx (HttpClientErrorException) and 5xx
                // (HttpServerErrorException) error coming from server side
                final HttpStatusCodeException httpException = (HttpStatusCodeException) ex;
                final DataErrorEnum error = DataErrorEnum.getErrorEnumByException(httpException);
                final String errorMessage = httpException.getResponseBodyAsString();
                dataFetcher.setErrorReponse(errorMessage, error);
            }
        }
    }
    

    It is so much simpler now.

    Finally, DataClient:

    public class DataClient implements Client {
    
        @Override
        public ListenableFuture<DataResponse> getData(final DataKey key) {
            final LinkedList<String> hostnames = getHostnames(key.getUserId());
            final Hosts hosts = new Hosts(hostnames);
            final SettableFuture<DataResponse> responseFuture = SettableFuture.create();
            final DataFetcher dataFetcher = new DataFetcher(responseFuture, key, hosts);
            dataFetcher.tryNextHost();
            return responseFuture;
        }
    
        ...
    } 
    

11. It seems quite good but you should test it: write unit tests for the refactored classes. If it's easy it might sign that it's a good design. If it's hard - that means that the code definitely needs further improvements. (Killing the Helper class, part two will help you.)

12. Finally, I would improve RetryCallback a little:

    if (ex instanceof SocketException) {
        // if it comes here, then it means some of the servers are down so
        // adding it into block list
        ShardMapping.blockHost(hostname);
        dataFetcher.tryNextHost();
    } else {
        ...
    }
    

    You could extract out an explanatory variable:

    final boolean serverDown = ex instanceof SocketException;
    if (serverDown) {
        ShardMapping.blockHost(hostname);
        dataFetcher.tryNextHost();
    } else {
    

    It eliminates the comment. See: Chapter 6. Composing Methods, Introduce Explaining Variable in Refactoring: Improving the Design of Existing Code by Martin Fowler; Clean Code by Robert C. Martin, G19: Use Explanatory Variables.