Add Caffeine cache layer extension#2417
Conversation
drcrallen
force-pushed
the
caffeineCache
branch
from
f0673ea to
f80ac22
Compare
|
The MapCache concurrency problems are a pain ( see #1836 ). Having a "local" cache that can be nice and performant would be awesome. |
|
This will fail under java7 tests as |
drcrallen
force-pushed
the
caffeineCache
branch
from
f80ac22 to
21f91a4
Compare
drcrallen
force-pushed
the
caffeineCache
branch
from
21f91a4 to
2d5ca25
Compare
| { | ||
| private static final Logger log = new Logger(CaffeineCache.class); | ||
| private final Cache<String, byte[]> cache; | ||
| private final AtomicReference<com.github.benmanes.caffeine.cache.stats.CacheStats> priorStats = new AtomicReference<>( |
There was a problem hiding this comment.
I would put this extension under client.cache.caffeine so that the package types don't take precedence. Then Droid's CacheStats could be fully qualified in only getStats() and the overall verbosity reduced.
There was a problem hiding this comment.
Moved around at https://github.com/metamx/druid-cache-caffeine
|
@drcrallen are we sure that this will fix the locking problems ? if yes can you explain or share some benchmarks ? |
|
@b-slim I have a high degree of confidence this will fix locking issues, but almost zero information on if this offers better latency than the guava cache. Luckily it is in an extension so it is completely optional. I want to get numbers aside from the general Caffeine benchmarks on the matter, but will not be able to immediately. |
|
@ben-manes One thing I could find a good way to handle was "namespace"s Does caffeine have any way to use a global retention rule, but divide the keys up into namespaces? |
|
No, but it might be possible to build something like that on top. It is a little vague of what that means and how generally useful it is. I've seen it discussed in contexts of other caches, but it was never requested for Guava's. Namespaces used to be asked for a lot back in memcached's heyday. Most of the time it was to invalidate a bunch of keys together which were inter-related. The solution was to use generational caching (a version number as part of the key) and increment the generation when the data store was updated. That relied on the cache to lazily evict the old keys which wouldn't be used anymore. Alternatively you could build indexes and rely on locking since this is part of the same process. A If namespaces are to make regions to have sub and global thresholds, this is even messier. That's usually to share a cluster of memcached and perform resource accounting (e.g. Google's internal rewrite did this) or have an automatic cache sizing. The former makes no sense here, the latter is very messy in Java where GC masks sizes. Attempts to do it automatically (soft references, heap monitoring, etc) don't work well. Its better to be explicit and move large scale caching outside of the JVM (off-heap, memcached, etc). So messy and confusing from my side of the fence. I think the useful hooks are exposed for a custom job, though. |
|
@ben-manes the big thing for use in druid is that the data queries have the underlying data segment as part of the cache key. As such, if the segment is no longer present on a node (in the case of using local cache) then that node will no longer receive queries for that segment, so any sort of LRU-like eviction policy should favor destroying those no-longer-used keys. The current methodology in this PR is similar to the generational approach you described, but the existing cache keys are scanned and invalidated in a best-effort manner. It could just fall back to lazy eviction if performance is a concern for actively scanning the keys. |
|
Thanks for the insight. |
|
If you know when a segment is no longer present then you could maintain an index as described above. Then when the segment is dropped you invalidate all of the associated keys. This could be done racy (depending on eviction to cleanup) or atomically (a bit of locking) depending on your preference. The atomic approach might use a CacheWriter (synchronous) or RemovalListener (asynchronous). The complexity to do it "correctly" and discard data proactively may not turn into a performance win, though. |
|
This has transient failures in the new UT. Please please try to reduce transient failures. |
|
@fjy failures in java7 UT are not transient. See master comment |
|
@drcrallen IMO the best way to resolve the java7 thing is to have this not be in the druid repo, but be in a different repo and be labeled as requiring java8. |
|
I have moved this over to https://github.com/metamx/druid-cache-caffeine since it requires java8 |
|
👍 |
6 participants
Tests are all based on #1937
TODO: