Egw upgrade #166
Egw upgrade #166
Conversation
…al cache with loading pattern
| * All the objects stored in the cache will be return without cloning or deep cloning due to improve the cache performance. | ||
| * Because of that user should be responsible of the mutability of the stored object. | ||
| */ | ||
| public final class AXPCache implements ICache |
There was a problem hiding this comment.
Occurred that it "may" be possible to support this implementation through javax.cache, by registering AXP as a separate caching provider. Will find more on this and confirm. Nothing to fix on this yet. This comment is to serve as just a note.
| purge(); | ||
| } | ||
| }; | ||
| Timer timer = new Timer("Timer"); |
There was a problem hiding this comment.
How about replacing this with a ScheduledExecutorService? That way the number of TimerThreads won't increase as the caches grow.
| @Override | ||
| public void run() | ||
| { | ||
| purge(); |
There was a problem hiding this comment.
Check if this is prone to a ConcurrentModificationException. If that's the case, timer task handling purge operation can stop.
There was a problem hiding this comment.
moved to ScheduledExecutorService. it resolves the problem.
| else | ||
| { | ||
| long expiryTime = System.currentTimeMillis() + periodInMillis; | ||
| cache.put(key, new SoftReference<>(new CacheObject(value, expiryTime))); |
There was a problem hiding this comment.
Using a SoftReference is fine, but I still think we need to clear HashMap entries using some algorithm. Like an LRU or a LFU algorithm. The reference held by SoftReference which is the CacheObject in this case will be removed, but how about the SoftReference itself?
There was a problem hiding this comment.
implemented another task to clean null references.
| */ | ||
| @Override | ||
| public <K, V> V getData(final AXPCache axpCache, final IAXPCacheLoader<K, V> cacheLoader, final K cacheKey) throws | ||
| Exception |
There was a problem hiding this comment.
Can we create a specific Exception here?
There was a problem hiding this comment.
Need to do a proper analysis of the existing exceptions and define a new hierarchy in the framework level. Until that is it safe to keep this as it is?
| <dependency> | ||
| <groupId>org.wso2.carbon.apimgt</groupId> | ||
| <artifactId>org.wso2.carbon.apimgt.rest.api.util</artifactId> | ||
| <version>6.0.4</version> |
There was a problem hiding this comment.
Version should be updated..
| userAdminStub._getServiceClient().getOptions().setProperty(HTTPConstants.CONNECTION_TIMEOUT, DEFAULT_CONNECTION_TIMEOUT); | ||
| } catch (AxisFault e) { | ||
| log.error("", e); | ||
| throw new BusinessException(GenaralError.INTERNAL_SERVER_ERROR); |
There was a problem hiding this comment.
Pass the original exception as an argument so that error is traceable in log file.
There was a problem hiding this comment.
Lifted change. will do the correction
|
|
||
| } catch (RemoteException | UserAdminUserAdminException e) { | ||
| log.error("UIPermission.build", e); | ||
| throw new BusinessException(GenaralError.INTERNAL_SERVER_ERROR); |
There was a problem hiding this comment.
Pass the original exception.
There was a problem hiding this comment.
lifted chnage. will do the correction
| } | ||
|
|
||
| public static UserRolePermissionFactory getInstance() { | ||
| if (instance == null) { |
There was a problem hiding this comment.
Prone to fail in multi-threaded scenarios. Can you use double-checked locking or eager initialization?
There was a problem hiding this comment.
Btw, noticed that this file appears as a change since you have formatted the code. But anyway its better to fix this now, the moment we noticed it.
There was a problem hiding this comment.
yes. these are lifted changes. didnt do any modifications. merged as it is. btw , will fix those
| @Override | ||
| public long size() | ||
| { | ||
| return cache.entrySet().stream().filter(entry -> Optional.ofNullable(entry.getValue()).map(SoftReference::get) |
There was a problem hiding this comment.
Each call to size would go through all entries to check expiry. Can't we simple return the size without explicitly checking for validity, and letting cleanup task do the eviction?
components/framework-axp/src/main/java/framework/cache/AXPCache.java
Outdated
Show resolved
Hide resolved
…and general cache with loading pattern, This reverts commit c1a3eaa
merge all changes from patch and release branches.