1

I am using Callable in my code which will be called by multiple threads as shown below. As of now, whenever any RestClientException is thrown then I am adding hostname to blockList.

public class Task implements Callable<DataResponse> {

    private DataKey key;
    private RestTemplate restTemplate;

    public Task(DataKey key, RestTemplate restTemplate) {
        this.key = key;
        this.restTemplate = restTemplate;
    }

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

        // construct what are the hostnames I can call basis on user id
        List<String> hostnames = some_code_here;

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

                // some code here to return the response if successful
            } catch (HttpClientErrorException ex) {
                // log exception
                return new DataResponse(errorMessage, error, DataStatusEnum.ERROR);
            } catch (HttpServerErrorException ex) {
                // log exception
                return new DataResponse(errorMessage, error, DataStatusEnum.ERROR);
            } catch (RestClientException ex) {
                // I don't want to add it to block list instantly.
                // If same hostname as failed five times consecutively, then only add it
                DataMapping.blockHost(hostname);
            }
        }

        return new DataResponse(DataErrorEnum.SERVER_UNAVAILABLE, DataStatusEnum.ERROR);        
    }
}

Below is what I have in DataMapping class:

private static final AtomicReference<ConcurrentHashMap<String, String>> blockedHosts = 
        new AtomicReference<ConcurrentHashMap<String, String>>(new ConcurrentHashMap<String, String>());

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

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

Problem Statement:-

Now as you can see in call method, I am blocking the hostname as soon as it throws RestClientException which may not be right. I need to see if a particular hostname has thrown RestClientException for five times consecutively, then only add this hostname to blockList by calling this line DataMapping.blockHost(hostname); otherwise don't add it to the blockList.

What is the most efficient and best way to do this? Maximum, I will have 70-100 unique machines in total.

In this case, my call method will be called from multiple threads so I need to make sure I am keeping the count properly for each hostname in case they throw RestClientException.

EDIT:

I also have below method in DataMapping class as well:

I have a background thread which runs every 2 minutes that replaces the entire set, since my service provides real data whether any hostname is really blocked or not. And I guess I do need the atomic reference for when I replace the entire set.

I am adding block feature locally in the code as well since I might be to know which machine is blocked after 2 minutes so it's better to know it before hand if possible.

// this is being updated from my background thread which runs every 2 minutes
public static void replaceBlockedHosts(List<String> hostNames) {
    ConcurrentHashMap<String, String> newBlockedHosts = new ConcurrentHashMap<>();
    for (String hostName : hostNames) {
        newBlockedHosts.put(hostName, hostName);
    }
    blockedHosts.set(newBlockedHosts);
}

2 Answers 2

1

I would associate each host with an AtomicInteger that is incremented on each RestClientException. This integer would be set to zero on a succesful call to enforce the "five consecutive times" constraint. The code would look something like this.

private final ConcurrentHashMap<String, AtomicInteger> failedCallCount = new ConcurrentHashMap<>();

void call() {
      try {
          String url = createURL(host);
          // make rest call
          resetFailedCallCount(host);
          // ...
      } catch (RestClientException ex) {
          registerFailedCall(host);
          if (shouldBeBlocked(host)) {
              DataMapping.blockHost(host);
          }
      }
}


private boolean shouldBeBlocked(String hostName) {
    AtomicInteger count = failedCallCount.getOrDefault(hostName, new AtomicInteger());
    return count.get() >= 5;
}

private void registerFailedCall(String hostName) {
    AtomicInteger newValue = new AtomicInteger();
    AtomicInteger val = failedCallCount.putIfAbsent(hostName, newValue);
    if (val == null) {
        val = newValue;
    }
    if (val.get() < 5) {
        val.incrementAndGet();
    }
}

private void resetFailedCallCount(String hostName) {
    AtomicInteger count = failedCallCount.get(hostName);
    if (count != null) {
        count.set(0);
    }
}

This is lock free (in our own code at least) and is very efficient. However it is vulnerable to some race conditions. Most notable the count can become larger than 5. However, that should not be a problem since the host is blocked anyways and the count is not used for anything else.

3
  • Thanks for your suggestion. I did got some idea from your suggestion. I was using AtomicReference on blockedHosts so that I can notify all my threads instantly if there is any change that was put in that map. I still need that right? And second question is, where do we unset the integer value - I mean let's say machineA throws RestClientException 4 times consecutively but 5th time, it started serving request, then the count should be set back to 0 right for that hostname? Where I will do that?
    – john
    Jun 6, 2015 at 19:21
  • @david AtomicReference is not needed on the ConcurrentHashMap as long as you do not re-assign that field? Changes inside of it will be visible to all threads immediately regardless. I would reset the count after a succesful call, i.e. after the line response = restTemplate.exchange(url, HttpMethod.GET, key.getEntity(), String.class); and do that every call (cheaper than inspecting if you should reset or not - just do it!). Jun 6, 2015 at 19:23
  • hey you still around? I am in this chat room. Have another basic question.
    – john
    Jun 7, 2015 at 16:33
0

Maintain a static register like - public static ConcurrentHashMap<String, Integer> toBeBlockedHostName = new ConcurrentHashMap<String, Integer>(); in your DataMapping class. And then use it like this your FOR loop:

  for (String hostname : hostnames) {

        // .. some code here
        //After ensuring everything is success and no RestClientException, i.e. can be last line of your TRY block...
        DataMapping.toBeBlockedHostName.remove("stackoverflow6361");
        catch (RestClientException ex) {
            if(DataMapping.toBeBlockedHostName.get("stackoverflow6361") == null){
                DataMapping.toBeBlockedHostName.put("stackoverflow6361", new Integer(1));
            } else{
                if(DataMapping.toBeBlockedHostName.get("stackoverflow6361") == 5){ //Don't hard code 5, have it from some property file after defining as retryThreshold...
                    System.out.println("Blocking threshold reached, block the hostname...");
                    DataMapping.blockHost(hostname);
                } else{
                    DataMapping.toBeBlockedHostName.put("stackoverflow6361", (toBeBlockedHostName.get("stackoverflow6361") + 1));
                }
            }
        }

Please note:: For ConcurrentHashMap even though all operations are thread-safe, retrieval operations do not entail locking.

Please pay attention, that after 5 consecutive retry fails, you will block the hostname, but if you are unblocking it again, then you should clear the register.

P.S.: Have appropriate getter and setter for HashMap.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.