Simplifying asynchronous "executeAsync" method along with "onFailure" callback

Question

I have a system in which user id is "sharded" across all the machines which means each machine is responsible for certain user id data. I am working on a library which will take DataKey builder object which contains user id, timeout, and some other stuff. I will be using this user id to figure out which machines have the data for this user id.

Customer will pass DataKey builder object to my executeAsync method and then I will use DataKey builder object to figure out the machines which has the data for this user id and then I will use those machines to make the call and get the data for them. For example, DataKey object has user id 12345 so below are steps which I will be executing.

• Get all the hostnames which will have the data for this user id 12345. It should be linked list.
• Now, pick the first machine from the hostnames list and generate the URL for that and execute the URL. If response is successful then make a DataResponse object and return the response and we will not log anything. But if the response is not successful like, server returned back any HttpClientErrorException or HttpServerErrorException, then I need to log it and make DataResponse object accordingly and return it.
• But let's say if the first machine in that list was down for some reason then I will log it that this machine is down and then I will try calling second machine in the hostnames list and if it is also down, then I will try third or fourth and keep doing the same thing. And suppose if all the machines are down in that hostnames list, then I will make DataResponse object saying all servers are down and log it as well.

We have three datacenters with 5 machines in each datacenters. And each user data is in two machines (primary and secondary) in each datacenters. Let's say if somebody is calling my library from DC1, then I will first call my primary machine server in DC1 only and depending on what flags they have enabled in DataKey, I will call other machines if they are down.

Below is the code I have so far which works fine but it is looking more messy as I am putting all the logic in executeAsync method.

public interface Client {

    // for asynchronous
    public ListenableFuture<DataResponse> executeAsync(DataKey key);
}

public class DataClient implements Client {

    // using spring 4 AsyncRestTemplate
    private final AsyncRestTemplate restTemplate = new AsyncRestTemplate();

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

        int shardId = getShardId(keys);

        String localDataCenterPath = FindDataCenterEnum.getLocalDataCenterString(keys.typeOfFlow());
        String remoteDataCenterPath = FindDataCenterEnum.getRemoteDataCenterString(keys.typeOfFlow());

        boolean remoteFlag = keys.isRemoteFlag(); // remote datacenter if primary and secondary locals are down
        boolean secondaryFlag = keys.isSecondaryFlag();

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

        // this will give me the mappings to find out which machine is responsible for the "shardId"
        ShardMappings mappings = MachineToShardMapping.getMappings(keys.typeOfFlow());

        String localPrimaryHostname = null;
        String remotePrimaryHostname = null;
        if (!DataUtils.isEmpty(mappings.primaryHostIdToPartitionMapping)) {
            String localPrimaryHostId = mappings.primaryHostIdToPartitionMapping.get(localDataCenterPath).get(shardId);
            localPrimaryHostname = mappings.hostIdToMachineMapping.get(localDataCenterPath).get(
                    Integer.parseInt(localPrimaryHostId));
            String remotePrimaryHostId = mappings.primaryHostIdToPartitionMapping.get(remoteDataCenterPath).get(shardId);
            remotePrimaryHostname = mappings.hostIdToMachineMapping.get(remoteDataCenterPath).get(
                    Integer.parseInt(remotePrimaryHostId));
        }

        String localSecondaryHostname = null;
        String remoteSecondaryHostname = null;
        if (!DataUtils.isEmpty(mappings.secondaryHostIdToPartitionMapping)) {
            String localSecondaryHostId = mappings.secondaryHostIdToPartitionMapping.get(localDataCenterPath).get(shardId);
            localSecondaryHostname = mappings.hostIdToMachineMapping.get(localDataCenterPath).get(
                    Integer.parseInt(localSecondaryHostId));
            String remoteSecondaryHostId = mappings.secondaryHostIdToPartitionMapping.get(remoteDataCenterPath).get(shardId);
            remoteSecondaryHostname = mappings.hostIdToMachineMapping.get(remoteDataCenterPath).get(
                    Integer.parseInt(remoteSecondaryHostId));
        }

        // now gather all the hostnames which has the data for user id 12345
        // depending on what flag is enabled
        if (remoteFlag && secondaryFlag) {
            hostnames.add(localPrimaryHostname);
            hostnames.add(localSecondaryHostname);
            hostnames.add(remotePrimaryHostname);
            hostnames.add(remoteSecondaryHostname);
        } else if (remoteFlag && !secondaryFlag) {
            hostnames.add(localPrimaryHostname);
            hostnames.add(remotePrimaryHostname);
        } else if (!remoteFlag && !secondaryFlag) {
            hostnames.add(localPrimaryHostname);
        } else if (!remoteFlag && secondaryFlag) {
            hostnames.add(localPrimaryHostname);
            hostnames.add(localSecondaryHostname);
        }

        // now I have machines in the hostnames  list so exeucute it
        executeForServers(responseFuture, keys, hostnames.get(0), hostnames.subList(1, hostnames.size()));
        return responseFuture;
    }

    /**
     * A simple method to execute each machine by making a URL for them. If one machine is down,
     * then it will execute other machine from the hostnames list.
     */
    private void executeForServers(final SettableFuture<DataResponse> responseFuture, final DataKey keys,
            final String hostName,  final List<String> restHostNames) {

        final org.springframework.util.concurrent.ListenableFuture orig =
            restTemplate.exchange(createURL(hostName, keys), HttpMethod.GET, keys.getEntity(), String.class);

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

                    // can we simplify this?
                    @Override
                    public void onFailure(Throwable ex) {
                        // this is for calling other machine if one machine is down
                        if (ex instanceof SocketException) {
                            if (restHostNames.isEmpty()) {
                                // all the machines are down
                                DataLogging.logErrors(ex, DataErrorEnum.SERVERS_DOWN, keys);
                                responseFuture.set(new DataResponse(null, DataErrorEnum.SERVERS_DOWN,
                                        DataStatusEnum.ERROR));
                            } else {
                                // this means one particular machine didn't responded so try another from the list
                                DataLogging.logErrors(ex, DataErrorEnum.SOCKET_EXCEPTION, keys);
                                executeForServers(responseFuture, key, restHostNames.get(0),
                                        restHostNames.subList(1, restHostNames.size()));
                            }
                        } else {
                            // this is for logging and making DataResponse object if we get status code like 4xx or 5xx
                            HttpStatusCodeException httpException = (HttpStatusCodeException) ex;
                            DataErrorEnum error = DataErrorEnum.getErrorEnumByException(httpException);
                            DataLogging.logErrors(ex, error, keys);
                            responseFuture.set(new DataResponse(httpException.getResponseBodyAsString(), error,
                                    DataStatusEnum.ERROR));
                        }
                    }
                });

        // propagate cancellation back to the original request
        responseFuture.addListener(new Runnable() {
          @Override public void run() {
             if (responseFuture.isCancelled()) {
               orig.cancel(false); // I am keeping this false for now
             }
          }
        }, MoreExecutors.directExecutor());
    }

    // this will generate the URL
    private String createURL(final String hostName, final DataKey keys) {
        StringBuilder url = new StringBuilder();
        // make a url here
        return url.toString();
    }

    // this will give me the shard id for user id "12345"
    private int getShardId(final DataKey keys) {
        // return the shard id for userId present in keys
    }
}

And below is my DataErrorEnum class which is being used mainly for reporting error :

public enum DataErrorEnum {
    OK(200, "NONE"),
    BAD_REQUEST(400, "Server Bad Request"),
    UNAUTHORIZED(401, " Server Unauthorized"),
    SERVER_TIMEOUT(408, "Server Timeout"),
    INTERNAL_SERVER_ERROR(500, "Internal Server Error"),
    SERVERS_DOWN(6000, "Servers Down"),
    ERROR_SERVICE(6002, "Random Error Occurred on Server"),
    SOCKET_EXCEPTION(6004, "Specific Server is Down");

    // constructor and getters

    public static DataErrorEnum getErrorEnumByException(HttpStatusCodeException httpException) {
        HttpStatus statusCode = httpException.getStatusCode();
        switch (statusCode) {
            case BAD_REQUEST:
                return DataErrorEnum.BAD_REQUEST;
            case INTERNAL_SERVER_ERROR:
                return DataErrorEnum.INTERNAL_SERVER_ERROR;
            case NO_CONTENT:
                return DataErrorEnum.NO_CONTENT;
            case OK:
                return DataErrorEnum.OK;
            case REQUEST_TIMEOUT:
                return DataErrorEnum.SERVER_TIMEOUT;
            case SERVICE_UNAVAILABLE:
                return DataErrorEnum.SERVERS_DOWN;
            case UNAUTHORIZED:
                return DataErrorEnum.UNAUTHORIZED;
            default:
                return DataErrorEnum.ERROR_SERVICE;
        }
    }
}

Is there any way to simplify my DataClient code a bit? I was thinking to separate the logic of getting the hostnames given the user id but not sure how should I separate them out and where do I put it? As of now my DataClient class is looking pretty big.

I am using AsyncRestTemplate as my async http client which will be async non blocking architecture.

Answer 1

1. If I'm right you could change

    final org.springframework.util.concurrent.ListenableFuture orig =
        restTemplate.exchange(createURL(hostName, keys), HttpMethod.GET, keys.getEntity(), String.class);

to

    final org.springframework.util.concurrent.ListenableFuture<ResponseEntity<String>> orig =
        restTemplate.exchange(createURL(hostName, keys), HttpMethod.GET, keys.getEntity(), String.class);

to avoid a compiler/Eclipse warning.

1. An operator would not be delighted with this error message:

ERROR_SERVICE(6002, "Random Error Occurred on Server"),

Is there something more helpful than "random"?

1. You have an extra space here:

UNAUTHORIZED(401, " Server Unauthorized"),

1. If you're using Guava, you could change

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

to

    import static com.google.common.collect.Lists.newLinkedList;
    [...] 
    List<String> hostnames = newLinkedList();

It's a little bit shorter and does not duplicate the type parameter.

1. You covered all possibilities here:

if (remoteFlag && secondaryFlag) {
} else if (remoteFlag && !secondaryFlag) {
} else if (!remoteFlag && !secondaryFlag) {
} else if (!remoteFlag && secondaryFlag) {
}

but I would add an else statement to be defensive (think about erroneous future modifications):

    } else {
        throw new IllegalStateException();
    }

It also makes the developer's intent clear. (The developer wanted to cover all possibilities, if execution goes to the last else branch it's definitely a bug.)

1. Instead of this subList:

executeForServers(responseFuture, keys, hostnames.get(0), hostnames.subList(1, hostnames.size()));

you could use an explanatory variable:

    final String firstHostname = hostnames.remove(0);
    executeForServers(responseFuture, keys, firstHostname, hostnames);

If you use a LinkedList reference (instead of list) it could be even more readable:

    final String firstHostname = hostnames.removeFirst();
    executeForServers(responseFuture, keys, firstHostname, hostnames);

See also: 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.

1. The code declares these booleans at the beginning:

boolean remoteFlag = keys.isRemoteFlag(); // remote datacenter if primary and secondary locals are down
boolean secondaryFlag = keys.isSecondaryFlag();

but uses them only at the end. You could move them closer to each other:

    boolean remoteFlag = keys.isRemoteFlag();
    boolean secondaryFlag = keys.isSecondaryFlag();
    if (remoteFlag && secondaryFlag) {

(The Principle of Proximity)

1. From Clean Code, Chapter 2: Meaningful Names:

Method Names

Methods should have verb or verb phrase names like postPayment, deletePage, or save. Accessors, mutators, and predicates should be named for their value and prefixed with get, set, and is according to the javabean standard.

I'd rename typeOfFlow() to getFlowType().

1. It's a little bit unclear:

DataKey keys

It is only one key (as the class name says) or multiple ones (as the variable name says)?

1. There are a few smells about these lines:

String localSecondaryHostId = mappings.secondaryHostIdToPartitionMapping.get(localDataCenterPath).get(shardId);
localSecondaryHostname = mappings.hostIdToMachineMapping.get(localDataCenterPath).get(
        Integer.parseInt(localSecondaryHostId));
String remoteSecondaryHostId = mappings.secondaryHostIdToPartitionMapping.get(remoteDataCenterPath).get(shardId);
remoteSecondaryHostname = mappings.hostIdToMachineMapping.get(remoteDataCenterPath).get(

    Integer.parseInt(remoteSecondaryHostId));

I guess I would create three separate instances of two classes from the ShardMappings class. But this might be a too big step. First, you should read this: Data Envy.

1. If I'm right primaryHostIdToPartitionMapping is a map with a list value type:

    public Map<String, List<String>> primaryHostIdToPartitionMapping;
   

Then move

String localPrimaryHostId = mappings.primaryHostIdToPartitionMapping.get(localDataCenterPath).get(shardId);
localPrimaryHostname = mappings.hostIdToMachineMapping.get(localDataCenterPath).get(
        Integer.parseInt(localPrimaryHostId));

to a getLocalPrimaryHostname method inside the ShardMappings class:

    public String getLocalPrimaryHostname(String localDataCenterPath, int shardId) {
        final String localPrimaryHostId = primaryHostIdToPartitionMapping.get(localDataCenterPath).get(shardId);
        final String localPrimaryHostname = hostIdToMachineMapping.get(localDataCenterPath).get(
                Integer.parseInt(localPrimaryHostId));
        return localPrimaryHostname;
    }

Do the same with the other three too:

• getRemotePrimaryHostname
• getLocalSecondaryHostname
• getRemoteSecondaryHostname

It increases encapsulation (since the data and the methods operating on/related to that data is in the same place).

1. Then you should notice that every method has similar structure. I would extract out a getHostname() method and use it from all of the four methods accordingly:

    public String getLocalPrimaryHostname(final String localDataCenterPath, final int shardId) {
        final String localPrimaryHostId = primaryHostIdToPartitionMapping.get(localDataCenterPath).get(shardId);
        final String localPrimaryHostname = getHostname(localDataCenterPath, localPrimaryHostId);
        return localPrimaryHostname;
    }
   
    private String getHostname(final String dataCenterPath, final String hostId) {
        final String hostname = hostIdToMachineMapping.get(dataCenterPath).get(
                Integer.parseInt(hostId));
        return hostname;
    }
   

2. Now my theoretical ShardMappings class looks like this:

    public class ShardMappings {
   
        public Map<String, List<String>> primaryHostIdToPartitionMapping;
        public Map<String, List<String>> secondaryHostIdToPartitionMapping;
        public Map<String, List<String>> hostIdToMachineMapping;
   
        public String getLocalPrimaryHostname(final String localDataCenterPath, final int shardId) {
            final String localPrimaryHostId = primaryHostIdToPartitionMapping.get(localDataCenterPath).get(shardId);
            final String localPrimaryHostname = getHostname(localDataCenterPath, localPrimaryHostId);
            return localPrimaryHostname;
        }
   
        public String getRemotePrimaryHostname(final String remoteDataCenterPath, final int shardId) {
            final String remotePrimaryHostId = primaryHostIdToPartitionMapping.get(remoteDataCenterPath).get(shardId);
            final String remotePrimaryHostname = getHostname(remoteDataCenterPath, remotePrimaryHostId);
            return remotePrimaryHostname;
        }
   
        public String getLocalSecondaryHostname(final String localDataCenterPath, final int shardId) {
            final String localSecondaryHostId = secondaryHostIdToPartitionMapping.get(localDataCenterPath).get(shardId);
            final String localSecondaryHostname = getHostname(localDataCenterPath, localSecondaryHostId);
            return localSecondaryHostname;
        }
   
        public String getRemoteSecondaryHostname(final String remoteDataCenterPath, final int shardId) {
            final String remoteSecondaryHostId = secondaryHostIdToPartitionMapping.get(remoteDataCenterPath).get(shardId);
            final String remoteSecondaryHostname = getHostname(remoteDataCenterPath, remoteSecondaryHostId);
            return remoteSecondaryHostname;
        }
   
        private String getHostname(final String dataCenterPath, final String hostId) {
            final String hostname = hostIdToMachineMapping.get(dataCenterPath).get(
                    Integer.parseInt(hostId));
            return hostname;
        }
    }
   

You can notice that

• hostIdToMachineMapping is used by only getHostname,
• primaryHostIdToPartitionMapping is used by only getLocalPrimaryHostname and getRemotePrimaryHostname and
• secondaryHostIdToPartitionMapping is used by only getLocalSecondaryHostname and getRemoteSecondaryHostname.

They're three separate groups, three separate responsibilities, they should have three classes. (Single responsibility principle)

You could create PrimaryShardMapping, SecondaryShardMapping and HostIdToMachineMapping classes here. Logic in PrimaryShardMapping and SecondaryShardMapping is almost the same, so you probably could generalize that into one class (maybe you need to use two factories to fill their maps properly).

Anyway, there could be other stuff in the ShardMappings class, so I stop here.

1. One more thing about ShardMappings: you could move the isEmpty test into the getLocalPrimaryHostname etc. methods and return null if the map is empty:

    public String getLocalPrimaryHostname(final String localDataCenterPath, final int shardId) {
        if (primaryHostIdToPartitionMapping.isEmpty()) {
            return null;
        }
        final String localPrimaryHostId = primaryHostIdToPartitionMapping.get(localDataCenterPath).get(shardId);
        final String localPrimaryHostname = hostIdToMachineMapping.getHostname(localDataCenterPath, localPrimaryHostId);
        return localPrimaryHostname;
    }
   

Then the value assignment of the hostname variables would be much simpler:

    String localPrimaryHostname = mappings.getLocalPrimaryHostname(localDataCenterPath, shardId);
    String remotePrimaryHostname = mappings.getRemotePrimaryHostname(remoteDataCenterPath, shardId);
    String localSecondaryHostname = mappings.getLocalSecondaryHostname(localDataCenterPath, shardId);
    String remoteSecondaryHostname = mappings.getRemoteSecondaryHostname(remoteDataCenterPath, shardId);

Note that using Optional is more expressive and less error-prone than nulls. Use either Optional from Java 8 or the one from Guava. (Optional in Guava, Using and avoiding null)

With Guava's Optional getLocalPrimaryHostname would look like this:

    public Optional<String> getLocalPrimaryHostname(final String localDataCenterPath, final int shardId) {
        if (primaryHostIdToPartitionMapping.isEmpty()) {
            return Optional.absent();
        }
        final String localPrimaryHostId = primaryHostIdToPartitionMapping.get(localDataCenterPath).get(shardId);
        final String localPrimaryHostname = hostIdToMachineMapping.getHostname(localDataCenterPath, localPrimaryHostId);
        return Optional.of(localPrimaryHostname);
    }

executeAsync:

    final Optional<String> localPrimaryHostname = mappings.getLocalPrimaryHostname(localDataCenterPath, shardId);

I like this because it says that "be aware, localPrimaryHostname can be absent" while you can so easily miss that with a plain String reference.

Unfortunately hostnames.add(localPrimaryHostname); won't work anymore. I guess I would create an addOptionalValue method (probably in a Collections3 class, since Collections2 is used by Guava):

    public static <T> void addOptionalValue(final Collection<T> collection, final Optional<T> value) {
        if (!value.isPresent()) {
            return;
        }
        collection.add(value.get());
    }

Then use it in the executeAsync method:

    addOptionalValue(hostnames, localPrimaryHostname);

1. It seems to me that the code uses to much helper methods. Killing the Helper class, part two is a great post on the topic.

1. The code uses these variables:

• restHostNames
• hostName
• localSecondaryHostname
• hostnames

It's not consistent whether n is uppercase or lowercase in hostname. I prefer the lowercase one, since hostname is rather one word in this context.

1. I've found that nested listener classes really hard to work with. Maintenance is a nightmare, they're almost impossible to properly test, especially when they call private methods of their top level class. I would break the dependencies here. Steps for this:

2. Create an interface (DataFetcher comes into my mind, it seems to be a good starting name but it does not feel perfect) for the executeForServers method.

3. Let DataClient implement DataFetcher and make its executeForServers public.

4. Convert your anonymous ListenableFutureCallback inner class to be a named nested class. (RetryCallback seems a good starting name.)

5. Make the nested class static and pass the DataClient instance (as a DataFetcher) to its constructor to make clear that DataFetcher is a dependency of the listener class.

6. Move RetryCallback to a separate file.

       public class RetryCallback implements ListenableFutureCallback<ResponseEntity<String>> {
           private final SettableFuture<DataResponse> responseFuture;
           private final LinkedList<String> restHostNames;
           private final DataKey keys;
           private final DataFetcher dataFetcher;
   
           private RetryCallback(final DataFetcher dataFetcher, final SettableFuture<DataResponse> responseFuture,
                   final LinkedList<String> restHostNames, final DataKey keys) {
               this.dataFetcher = checkNotNull(dataFetcher);
               this.responseFuture = checkNotNull(responseFuture);
               this.restHostNames = checkNotNull(restHostNames);
               this.keys = checkNotNull(keys);
           }
   
           ...
   
           @Override
           public void onFailure(final Throwable ex) {
               ...
               dataFetcher.executeForServers(responseFuture, keys, nextHostname, restHostNames);
           }
       }
   
       public interface DataFetcher {
           void executeForServers(SettableFuture<DataResponse> responseFuture, DataKey keys, String hostname,
                   LinkedList<String> restHostnames);
       }
   
       public class DataClient implements Client, DataFetcher {
   
           ...
   
           @Override
           public void executeForServers(final SettableFuture<DataResponse> responseFuture, final DataKey keys,
               final String hostName, final LinkedList<String> restHostNames) {
               ...
               final ListenableFutureCallback<ResponseEntity<String>> callback = new RetryCallback(this,
                   responseFuture, restHostNames, keys);
               ...
           }
       }
   

It makes dependencies explicit, helps to avoid spaghetti-code (you will see in your constructor if you have too much dependencies/parameters), makes testing easier (you can pass mocks instead of real implementations easily) as well as reading and maintenance (explicit dependencies clears who calls who).

You could find this technique and others useful ones in Michael C. Feathers great Working Effectively with Legacy Code book.

Answer 2

If I'm having the logic correctly, I believe you can move from this:

if (remoteFlag && secondaryFlag) {
    hostnames.add(localPrimaryHostname);
    hostnames.add(localSecondaryHostname);
    hostnames.add(remotePrimaryHostname);
    hostnames.add(remoteSecondaryHostname);
} else if (remoteFlag && !secondaryFlag) {
    hostnames.add(localPrimaryHostname);
    hostnames.add(remotePrimaryHostname);
} else if (!remoteFlag && !secondaryFlag) {
    hostnames.add(localPrimaryHostname);
} else if (!remoteFlag && secondaryFlag) {
    hostnames.add(localPrimaryHostname);
    hostnames.add(localSecondaryHostname);
}

To this:

hostnames.add(localPrimaryHostname);
if(remoteFlag) {
    hostnames.add(remotePrimaryHostname);
    if(secondaryFlag) { 
        hostnames.add(remoteSecondaryHostname);
    }
}
if(secondaryFlag) {
    hostnames.add(localSecondaryHostname);
}

---

You don't need your hostIds in String representation so you might as well immediately parse them:

String localPrimaryHostId = mappings.primaryHostIdToPartitionMapping.get(localDataCenterPath).get(shardId);
localPrimaryHostname = mappings.hostIdToMachineMapping.get(localDataCenterPath).get(Integer.parseInt(localPrimaryHostId));

becomes

int localPrimaryHostId = Integer.parseInt(mappings.primaryHostIdToPartitionMapping.get(localDataCenterPath).get(shardId));
localPrimaryHostname = mappings.hostIdToMachineMapping.get(localDataCenterPath).get(localPrimaryHostId);

Answer 3

Extract out the mapping code into its own function. There is a bit of a code smell with having to perform Integer.parseInt when you're not reading input or doing any sort of direct serialization, you may want to investigate that. Ideally, such a thing should be a much faster lookup and not involve any kind of calculation.

It is possible for the hostnames to be null, so your hostnames.add(...) calls have the capability to add null elements to your list, this is something else to be careful of.

The logic for this:

 if (remoteFlag && secondaryFlag) {
      hostnames.add(localPrimaryHostname);
      hostnames.add(localSecondaryHostname);
      hostnames.add(remotePrimaryHostname);
      hostnames.add(remoteSecondaryHostname);
  } else if (remoteFlag && !secondaryFlag) {
      hostnames.add(localPrimaryHostname);
      hostnames.add(remotePrimaryHostname);
  } else if (!remoteFlag && !secondaryFlag) {
      hostnames.add(localPrimaryHostname);
  } else if (!remoteFlag && secondaryFlag) {
      hostnames.add(localPrimaryHostname);
      hostnames.add(localSecondaryHostname);
  }

... can be clarified/simplified to:

 hostnames.add(localPrimaryHostname);

 if (secondaryFlag) {
     hostnames.add(localSecondaryHostname);
 }

 if (remoteFlag) {
     hostnames.add(remotePrimaryHostname);

     if (secondaryFlag) {
         hostnames.add(remoteSecondaryHostname);
     }
 }

Also, you suggested there may be more than one remote; perhaps there should be a loop and associated code to add all remotes, not just one of them.

As for simplifying your execution, from a high-level perspective, I'd remove the 'on failure' logic to try and chain your tasks. Instead encapsulate the 'talk to a host' as a single subtask. You can then have an orchestrating task that attempts one of those subtasks against a host and creates the next one if they fail. As some very crude pseudo-code:

 HostOperationTask extends Task {
     String hostname;
     Result run() {
         // do connecting to host things
     }
 }

 ResilientHostOperationTask extends Task {
     List<String> hostnames; // ensure correctly sorted...

     Result run() {
         bool success = false;
         do {
              String hostname = hostnames.pop(); // ... so they're consumed correctly
              execute(new HostOperationTask(hostname), new Callback() {
                   void onSuccess() { success = true }
                   void onFailure() { // logging etc.}
              }); // either execute synchronously or await/block for the result
         } while (hostnames.length() > 0 && !success);

         // react to success or not, cleanup, etc.