Conversation
onobc
left a comment
onobc
left a comment
There was a problem hiding this comment.
Thanks @christophstrobl . I have added some comments / questions.
| /** | ||
| * @author Christoph Strobl | ||
| */ | ||
| interface CacheWriterOperation<T> { |
There was a problem hiding this comment.
Can be a @FunctionalInterface.
| /** | ||
| * @author Christoph Strobl | ||
| */ | ||
| public abstract class ResetCachesStrategies { |
There was a problem hiding this comment.
I think this may read better as CacheResetStrategy/ies - wdyt?
Uggh, naming... always hard.
|
|
||
| private <T> T execute(String name, Function<RedisConnection, T> callback) { | ||
| @Override | ||
| public <T> T execute(Function<RedisConnection, T> callback) { |
There was a problem hiding this comment.
I am sure this will clear up w/ javadocs once we get firm on the direction, but it is currently not clear that the newly added execute method does not do the potential wait for unlocked (i.e. checkAndPotentiallyWaitUntilUnlocked).
| return execute(null, callback); | ||
| } | ||
|
|
||
| private <T> T execute(@Nullable String name, Function<RedisConnection, T> callback) { |
There was a problem hiding this comment.
We could just call executeLockFree instead of adding the @Nullable here.
| @Override | ||
| public void resetCaches() { | ||
|
|
||
| if(resetCachesStrategy instanceof CacheWriterOperation<?> operation) { |
There was a problem hiding this comment.
My first thought was to abstract the impl down into the strategy via a resetCaches method on the strategy where the strategy could take in a cache writer and a cache manager. The flushing variant could use the cache writer and the default variant could use the CM resetCaches impl. However, the latter would call back into CacheManager.resetCaches and that would infinite recurse (i.e. not get to super.resetCaches).
Just putting my thoughts - I am not sure which direction to go but I do like the thought of the strategy handling the work.
There was a problem hiding this comment.
I am sure I am missing something, but it seems that we are overlapping w/ current cache cleaning impl in BatchStrategy (keys and scan). Seems like we could just add another strategy for flushAsync().
| @Override | ||
| public String doWithCacheWriter(RedisCacheWriter cacheWriter) { | ||
| return cacheWriter.execute(connection -> { | ||
| connection.serverCommands().flushDb(FlushOption.ASYNC); |
There was a problem hiding this comment.
Is there any other cleanup / stats we are missing that the super.resetCaches may be doing that we should do here as well?
| /** | ||
| * @author Christoph Strobl | ||
| */ | ||
| public abstract class ResetCachesStrategies { |
There was a problem hiding this comment.
How about just ResetStrategy? Cache defines clear and evict methods and reset is distinct from these (thus it ranges semantically in a similar space).
Design-wise, it could be an interface with two static methods, sequential() (the default) and flushDb(). I like the CacheWriterOperation approach as it repeats how we approach functional composition in other parts of the framework.
Rename ResetCachesStrategies to ResetStrategy, introduce clear and invalidate strategies. Add tests and docs.
mp911de
changed the title
4 participants
No description provided.