OF-3084 OF-3088: Refactor Cache Locking implementation#2811
Conversation
Based on the single responsibility principle, the CacheFactory should not be managing Locks. Instead, Caches should do that themselves. Back in 2019, the method in CacheFactory that generates locks was marked as deprecated because of that, with a removal note in the distant future: 5.0.0. As 5.0.0 is upon us, the deprecated code has now been removed. The cache locking implementation has been moved to a dedicated class (`LocalCache`) that is used by caches directly. This commit attempts to achieve the desired removal of deprecated methods with a minimum of refactoring. Further improvements may be desired (e.g. not storing locks for all caches in one shared data structure, but rather in a per-cache data structure).
|
I've prepared a corresponding change in the Hazelcast plugin here: igniterealtime/openfire-hazelcast-plugin#115 |
|
I've smoke-tested the clustered cache modifications with the https://github.com/surevine/openfire-docker-compose/ project. |
|
As I'd like to dogfood this test, I'm merging this without waiting for a review. Don't let that stop anyone from reviewing after-the-fact. :) |
GregDThomas
left a comment
GregDThomas
left a comment
There was a problem hiding this comment.
A couple of hopefully helpful comments too
| } | ||
|
|
||
| private ReentrantLock lookupLockForAcquire(CacheKey cacheKey) { | ||
| CacheKey mutex = interner.intern(cacheKey); // Ensure that the mutex used in the next line is the same for objects that are equal. |
There was a problem hiding this comment.
I might be tempted to put this behind a getRepresentativeInstance(cacheKey) method, or similar, as you've done it twice already. Possibly as a method of CacheKey.
There was a problem hiding this comment.
I've read this three times but I still don't get what you mean :)
There was a problem hiding this comment.
Sorry - let me be more explicit.
I think this logic belongs in CacheKey and not LocalLock. So .. add a method to CacheKey
public CacheKey getRepresentativeInstance() {
return interner.intern(this);
}and invoke that from here (and the other place).
Taking that further, I might also be tempted to make the constructor private, and add a factory method to CacheKey ...
public static CacheKey createCacheKey(Cache cache, Object key) {
final CacheKey instance = new CacheKey(cache, key);
return interner.intern(CacheKey);
}So it's impossible to obtain anything other than the representative instance of the class.
There was a problem hiding this comment.
Ah, now I see. I've opened a new PR for this: #2821
| */ | ||
| class CacheKey | ||
| { | ||
| final String cacheName; |
There was a problem hiding this comment.
That'd probably lead to introducing a getter and setter for that field As this is package protected, having direct access to the field is an acceptable compromise maybe? It's slightly less code (for something that I don't expect to be used much anyway).
2 participants
Based on the single responsibility principle, the CacheFactory should not be managing Locks. Instead, Caches should do that themselves.
Back in 2019, the method in CacheFactory that generates locks was marked as deprecated because of that, with a removal note in the distant future: 5.0.0.
As 5.0.0 is upon us, the deprecated code has now been removed. The cache locking implementation has been moved to a dedicated class (
LocalCache) that is used by caches directly.This commit attempts to achieve the desired removal of deprecated methods with a minimum of refactoring.
Further improvements may be desired (e.g. not storing locks for all caches in one shared data structure, but rather in a per-cache data structure).