asked on

improving cache design

hi guys

I have somethng like this

public class CacheImpl{

Map<String,CacheData> localCache = new HashMap<String,CacheData>();

private void cacheImpl(){
String webserviceId = getWebserviceId(); //returns a uniqe webserviceId ;
cacheMap.put(webserviceId , new CacheData());
Map cache = construct(cacheMap);
}


private Map construct(Map cacheMap){
CacheData newCData = null;
//get webserviceId from cacheMap, make a webservice call and return CacheData
for(String webserivceId : cacheMap.keySet()){
 newCData = webservicecall(webserivceId);
}
 cacheMap.put(webserviceId , newCData);  
 localCache.putAll(cacheMap); //put cacheMap in localCache
}

}//end class

Open in new window

The idea is to first check the localCache, it the key already exists , use the value .
If not, make a call to webservice, so we have a check like this in cacheImpl()

private void cacheImpl(){
String webserviceId = getWebserviceId(); //returns a uniqe webserviceId ;

if (localCache.get(webserviceId)!= null){
CacheData = localCache.get(webserviceId);
}
else{
// make webservice call
cacheMap.put(webserviceId , new CacheData());
Map cache = construct(cacheMap);
}
}

Open in new window

Question 1. Is this a good design?

The idea is also to clean up the localCache once a day so we have another method which does that

	@Scheduled(cron = "${localCacheCleanup}")  //localCacheCleanup runs at 12.00 AM everyday
	public void localCacheCleanup() throws InterruptedException {		 
			localCache.clear(); 
			Question 2:
			//Will there be a provlem here? Lets say one thread (from executor serivice thread pool is already using localCache
			//while the scheduler thread removes the data from localCache. Any sugessions how to improve the design)
		 
	}

Open in new window

thanks

SOLUTION

girionis

membership

This solution is only available to members.

To access this solution, you must be a member of Experts Exchange.

Start Free Trial

dpearson

This design is basically fine for a single threaded application.

However, if you are supporting multiple threads using the same cache (which it sounds like you are) then this design isn't quite right. The problem is that HashMap doesn't support multiple threads operating on the same map well. You can get "ConcurrentModificationExceptions" and other nasty problems if one thread modifies the map while another is reading from it.

The solution is very simple - move to using:
ConcurrentHashMap<String, CacheData>

instead of HashMap. The rest of your code won't need to change, and you'll avoid a lot of ugly issues.

Also I think this piece of the code is actually wrong:

CacheData newCData = null;
//get webserviceId from cacheMap, make a webservice call and return CacheData
for(String webserivceId : cacheMap.keySet()){
 newCData = webservicecall(webserivceId);
}
 cacheMap.put(webserviceId , newCData);

Open in new window

That will only record the last newCData value. The rest are discarded.

Think it should be:

//get webserviceId from cacheMap, make a webservice call and return CacheData
for(String webserivceId : cacheMap.keySet()){
 CacheData newCData = webservicecall(webserivceId);
 cacheMap.put(webserviceId , newCData);  
}
...

Open in new window

Doug

Jay Roy

ASKER

thanks Doug. I am still a little doubtful if just changing HashMap<String, CacheData>
to ConcurrentHashMap<String, CacheData> alone will solve the problem.

Lets say one executor service thread is executing this line
>> CacheData = localCache.get(webserviceId);

while the scheduler thread executes this line
>>localCache.clear();

and both above operations occur at the exact same time with some unlucky timing, then that would be a issue although i am using ConcurrentHashMap.

Please correct me if i am wrong.

Thanks.

dpearson

Those two operations should be fine.

It's sort of the goal of ConcurrentHashMap. One thread can be reading from it (localCache.get()) which the other thread is writing to it (localCache.clear()).

Whether the thread doing the "get" sees an empty bucket or not would depend on whether the "clear" had reached that bucket of the hashmap or not, but either way the behavior is valid. If both threads are executing at the same time, the get could return either the cached value or an empty value, but it would not error out. Same thing for the clear - it would not be blocked or trigger an error if it was trying to clear the bucket in the map that the get was actively trying to read.

And once the clear had completed, the next request to the cache would definitely get an empty result.

So I think it's all OK.

Doug

Jay Roy

ASKER

Thanks.

>>>the get could return either the cached value or an empty value, but it would not error out.

Agree, i think the problem would be here

if (localCache.get(webserviceId)!= null){ -->thread 1 comes to this line here and localCache is not cleared yet
CacheData = localCache.get(webserviceId); -->now when thread1 comes to this line and if localCache is cleared (by thread2), there is a potential issue
}                                         --> either a Null pointer exception will be thrown or it will return  empty value and call to 
else{                                     --> webservice will be skipped since i am already inside the if() condition.Sicne the localCache has been 
// make webservice call                   --> cleared out i would actually like to make a call to the webservice in the else section
cacheMap.put(webserviceId , new CacheData());
Map cache = construct(cacheMap);
}
}

Open in new window

ASKER CERTIFIED SOLUTION

dpearson

membership

This solution is only available to members.

To access this solution, you must be a member of Experts Exchange.

Start Free Trial