Client-side caching by command arguments #3698
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## 5.2.0 #3698 +/- ##
============================================
- Coverage 75.64% 75.46% -0.18%
Complexity 4962 4962
============================================
Files 298 300 +2
Lines 15113 15162 +49
Branches 1138 1141 +3
============================================
+ Hits 11432 11442 +10
- Misses 3178 3216 +38
- Partials 503 504 +1 ☔ View full report in Codecov by Sentry. |
| public final void invalidateKeys(List list) { | ||
| protected abstract Object get(long hash); | ||
|
|
||
| final void invalidateKeys(List list) { |
There was a problem hiding this comment.
Missing type parameter for List. I have seen this in several places around the code, it would be great to use generics consistently, if we can.
There was a problem hiding this comment.
Its only usage is in
where it is programmatically possible to ensure generics. The other things we can consider areObject or List<Object>. So I have been a bit less thoughtful here.
| } finally { | ||
| writeLock.unlock(); | ||
| } | ||
| } |
There was a problem hiding this comment.
What if there are cached values for this key? Should we remove them, given that the key was removed on the server, most probably? Should there be an else for the if (value != null)?
There was a problem hiding this comment.
Do you mean, what if there are cached values for this "command"? In that case we have already returned the result in Line#70.
There was a problem hiding this comment.
You're right, I had "command" in mind, not "key", and if there is anything cached, you don't even end up in this code.
Now that I re-read this, this might be a clean-up scenario: you have a command that is not cached, read from the server, get null, you could clean up potential other commands that were cached for the same key(s).
There was a problem hiding this comment.
It seems redundant as it should be covered by void invalidate(List list) (previously void invalidateKeys(List list)).
| public ClientSideCache() { | ||
| this.cache = new HashMap<>(); | ||
| protected ClientSideCache() { | ||
| this.keyHashes = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
It seems to me that we are only adding stuff to this map. Some more housekeeping will be needed, otherwise the map will only grow, we need to remove unused keys and stuff, at some point. Could this be in itself a Cache, either Caffeine, or Guava?
There was a problem hiding this comment.
Right now, I'm not sure which would be a better approach for clean-up. I'm not thinking about this ATM as I hope it can be done without effecting public APIs.
There was a problem hiding this comment.
Adding TODO comments for now.
| import java.util.concurrent.TimeUnit; | ||
| import redis.clients.jedis.ClientSideCache; | ||
|
|
||
| public class CaffeineCSC extends ClientSideCache { |
There was a problem hiding this comment.
Just a thought that comes to my mind when I read this: the general advice is to favor composition over inheritance. Maybe we could model this via composition, put an interface on top of CaffeineCSC and GuavaCSC and inject that interface into ClientSideCache?
There was a problem hiding this comment.
ClientSideCache itself is that interface (abstract class). It is an optional parameter for our actual client classes.
TODO: Compute hash code of command arguments.
Currently superseded by #3700