Adding hostname to block list after x consecutive failures in multithreading application

I am using Callable in my code which will be called by multiple threads as shown below. Given a user id in DataKey object, I will find out what are the hostnames I can use to get the data and then I will iterate that hostnames list one by one to get the data.

As of now, whenever any RestClientException is thrown on a particular hostname then I am adding that hostname to blockList so that no other thread can make a call to that hostname.

```
public class Task implements Callable {

private final DataKey key;
private final RestTemplate restTemplate;

private final ConcurrentHashMap failedCallCount = new ConcurrentHashMap<>();

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

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

// construct what are the hostnames I can call basis on user id
List 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);

resetFailedCallCount(hostname);

// 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) {
registerFailedCall(hostname);
if (

Can we simplify that part and may be do everything in DataMapping class?

Sure, but what Data and what Mapping? There's neither, it's a BlockingManager or whatever.

As it stands, you could simply move the code there and be done. However, static is mostly bad and you should create an instance and it do the work (you could use the singleton (anti-)pattern or something better).

You're using the map in blockedHosts as a Set. This is fine, but why to put there some dummy data when you can put there the counters?

So I'd combine and reduce it to something like

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

boolean isBlocked(String hostName) {
    AtomicInteger count = blockedHosts.get();
    return count != null && count.get() >= BLOCKING_THRESHOLD;
}

void onFailure(String hostname) {
    AtomicInteger newValue = new AtomicInteger();
    AtomicInteger val = blockedHosts.get().putIfAbsent(hostname, newValue);
    // no need to care about over-reaching 5 here
    (val == null ? newValue : val).incrementAndGet();
}

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

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

I'm pretty sure, that you need no AtomicReference as volatile would suffice. You could also use Guava's AtomicLongMap which is just like ConcurrentHashMap but simplifies the operations you need.

StackExchange Code Review Q#92859, answer score: 2