LGTM

Alternatively, ConcurrentLinkedHashMap is a concurrent, LRU cache that supports weighted entries. That might be a good stop-gap to offer much of the Caffeine-based extension's performance, except for the improved eviction policy. The cache was fairly successful, e.g. adopted by Cassandra and ported into Guava (though at a large perf loss). It is also Java 6 compatible.

@ben-manes Thanks for the suggestion. Is it easy to implement the eviction policy in ConcurrentLinkedHashMap?

CLHM is an LRU policy on top of ConcurrentHashMap. Its intentionally bare bones because it took me a long time to stumble on the right design ideas. The challenge with LRU is that every read is a write to global, shared state. The solution is to use a database style commit log to record and replay events. We then tried to retrofit the ideas onto Guava and I rewrote it in Caffeine.

So if you're asking if its easy to use here for an LRU cache, then yes.

ConcurrentMap<K, V> cache = new ConcurrentLinkedHashMap.Builder<K, V>()
    .maximumWeightedCapacity(1000)
    .weigher(...)
    .build();

If you're asking if its easy to implement the ideas from scratch, then its moderate. But doing it with all the features and tuning in Caffeine was hard. However that also replaced LRU with a modern policy, which is one of the reasons why Druid 0.9.2 adopted it as an extension. Maybe when Druid is JDK8-based they'll make it the default.

IMHO we shouldn't try to reinvent the wheel here. The local cache in druid has tons of problems that make it unsuitable once you get to any appreciable scale, which is why I took @ben-manes 's advice and checked out Caffeine, which is working great for us.

The simple local cache does, however, provide a great way to spin up a mild druid cluster with minimal overhead.

I'm not saying it shouldn't be fixed, but I think focus should be on getting the default local cache more functional and usable, rather than trying to write the-next-big-cache.

@drcrallen oh I don't think anyone is suggesting that :)

It was unclear whether @xiaoyao1991's question was about usage or implementation. LHM is extended to become a cache, whereas CLHM simply is one (as any other form of a concurrent LHM makes little sense). But I suspect he meant the former rather than the internal details.

Anyway, I think @xiaoyao1991's changes are good and wanted to suggest CLHM as an option if Druid needs to remain JDK7-based.

Ok, so this foregoes the LinkedHashMap's remove eldest entry hook in order to ensure the cache size never exceeds the configured value in the put call itself. Or more precisely, it will evict until either there is enough room or the cache is empty.

Thread safety is retained through io.druid.client.cache.MapCache#clearLock , but the contract for LinkedHashMap already mandates external synchronization. So that's ok.

Looks good from here.

👍

it's not letting me start and approve a review :-P

Thanks for the contrib @xiaoyao1991 , would you mind filling out the Druid CLA? http://druid.io/community/cla.html

@drcrallen he should be covered already

@drcrallen yes, the idea is just like what you said. Thread safety is guaranteed through the synchronized map.

I've just filled out the CLA.

Need me to squash the commits?

nope! github will squash them