1
\$\begingroup\$

This is an extension to this question

I am working on a library which makes Http call to my service. I have below two requirements:

  • If my service machine doesn't respond back (there is a socket timeout or connection timeout), I add them to my local blockList and if service machine doesn't respond 5 times consecutively then I don't make call to them. So let's say if machineA is not responding (throwing RestClientException), I will call onFailure method everytime and keep incrementing the counter and then while making call to machineA again, I check isBlocked method by passing machineA as hostname and 5 as threshold so if machineA has been blocked 5 times consecutively then I don't make call to them at all. If machineA is blocked (since its blocking count is >= 5) then I want to keep it block for a particular period of time. So I have another parameter which will tell us for how long I want to keep this machine blocked and after that interval is elapsed, then only I will start making call to them.
  • I have a background thread which runs every 2 minutes and it calls another different service which provides what machines are going to down and from this background thread, I call replaceHosts method to atomically update all the block machines so that I don't make calls to them.

Now to handle above two cases, I am using two different variables just to see what machines are blocked. See below DataMapping class:

Below is what I have in DataMapping class: Here value of threshold variable is 5 and interval is 5 minutes.

// do I need to use two variables here just to see which hosts are blocked?
// I guess we can simplify this and just have only one variable to check
// which ones are blocked
public static volatile ConcurrentHashMap<String, AtomicInteger> blockedFlowHosts =
  new ConcurrentHashMap<String, AtomicInteger>();
public static volatile ConcurrentHashMap<String, AtomicInteger> blockedHosts =
      new ConcurrentHashMap<String, AtomicInteger>();

private final ScheduledExecutorService scheduler = Executors.newSingleThreadScheduledExecutor();

// using both the variables to see which machines are blocked
public static boolean isBlocked(String hostname, int threshold) {
    AtomicInteger value = blockedFlowHosts.get(hostname);
    AtomicInteger count = blockedHosts.get(hostname);
    return ((count != null && count.get() >= threshold) || (value != null && value.get() >= threshold));
}

public static void onFailure(String hostname, int threshold,
  long interval) {
    AtomicInteger newValue = new AtomicInteger();
    AtomicInteger val = blockedHosts.putIfAbsent(hostname, newValue);
    int count = (val == null ? newValue : val).incrementAndGet();
    // the test here is `==` to make sure the task is scheduled only once
    if (count == threshold) {
        scheduler.schedule(new Runnable() {
            @Override public void run() {
                blockedHosts.remove(hostname);
            }
        }, interval, TimeUnit.MINUTES);
    }
}

public static void onSuccess(String hostname) {
    blockedHosts.remove(hostname);
}

// this is called from background thread which runs every two minutes and it replaces
// all blocked machines with new blocked machines or empty
public static void replaceHosts(List<String> blockList) {
    ConcurrentHashMap<String, AtomicInteger> newBlockedHosts = new ConcurrentHashMap<>();
    for (String hostname : blockList) {
      newBlockedHosts.put(hostname, new AtomicInteger(Byte.MAX_VALUE));
    }
    blockedFlowHosts = newBlockedHosts;
}   

Below is my main thread code where I am using DataMapping methods to check whether hostname is blocked or not and also to block hostnames.

@Override
public DataResponse call() {
    ResponseEntity<String> response = null;

    List<String> hostnames = some_code_here;

    for (String hostname : hostnames) {
        // If hostname is in block list, skip sending request to this host
        if (DataMapping.isBlocked(hostname)) {
            continue;
        }
        try {
            String url = createURL(hostname);
            response = restTemplate.exchange(url, HttpMethod.GET, null, String.class);
            DataMapping.onSuccess(hostname);

            // some code here to return the response if successful
        } catch (RestClientException ex) {
            // adding to block list
            DataMapping.onFailure(hostname);
        }
    }

    return DataResponse here
}

Problem Statement:-

Currently, my logic boils down to, "If I've detected too many failures, OR I've been told explicitly, don't use the host." One collection is for self-monitoring, the other is managed independently through replaceHosts().

Do I need to have two different variables blockedFlowHosts and blockedHosts to figure out which machines are blocked? Can we not use only one variable? I'd like to know if there is any way by which I can simplify the above code to just have one variable which has all the info for blocked hosts?

\$\endgroup\$
  • \$\begingroup\$ I guess, that as long as you want exactly this behavior, using two variables is best. If you could change the condition to something like count + value >= 5, it might work with a single variable. \$\endgroup\$ – maaartinus May 17 '16 at 21:40
  • \$\begingroup\$ I didn't quite follow that? How can I use single variable? \$\endgroup\$ – arsenal May 17 '16 at 21:42
  • \$\begingroup\$ I only mean, that if you can change the condition, then it might be possible. It may or may not be possible to manage the sum in a single variable, but without much thinking I can't see how. That's a sign that it's probably not worth it. Two variables are straightforward and clear and I'd leave it this way. Briefly looking through you code, the worst thing I've found are the local variable names in isBlocked(....); IOW the code is fine. \$\endgroup\$ – maaartinus May 17 '16 at 21:50
  • \$\begingroup\$ Yeah l was not sure what to put local variables name in the isBlocked method. Do you have any suggestions on that? \$\endgroup\$ – arsenal May 17 '16 at 21:56
  • \$\begingroup\$ Not really, but I can't see why you're associating blockedFlowHosts <-> value and blockedHosts <-> count and not e.g. the other way round. The names don't matter much as the method is very short, but I hate strange associations. So value1 and value2 would be IMHO much better (they're interchangeable, anyway). Even better than giving them a meaning like valueFlow and valueWhatever. \$\endgroup\$ – maaartinus May 17 '16 at 22:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.