Move caffeine out of extension and make it the default cache implementation. by drcrallen · Pull Request #4810 · apache/druid

drcrallen · 2017-09-15T22:33:18Z

The caffeine library itself is actually something that could be used outside of just the extension. This allows other things to pull in caffeine, as well as puts caffeine as a core cache in druid without the need for an extension. This also sets Caffeine as the default cache and deprecates local cache.

The setting for druid.cache.sizeInBytes should transparently carry over to the new cache, but the other two settings for local cache do not have analogs in the caffeine cache.

ben-manes · 2017-09-15T23:01:19Z

I think that newer Caffeine versions are better for the JRE bug, but definitely it is not ideal. Previously the write buffer was unbounded, so when investigating we switched to being bounded (planned, but hadn't been a priority). Then writers spun waiting for the state condition, assuming the task was executing or queue'd to run, and a small busy wait would be good enough. But a full executor, lost task, write storms, etc. made that a poor idea.

Now the writers help out by busy waiting and then falling back to perform the maintenance work (acquire the lock). This improves write throughput in a synthetic load by descheduling threads and lets progress occur if the executor has been swamped (so our task can't get run). For lost tasks before 8u60, it gets reset by the writer doing the work. So this solves the problem, but means the write buffer has to be full before the back pressure kicks in. That could be good enough, but does mean the cache would exceed capacity as write notifications queue'd up and delayed eviction.

drcrallen · 2017-09-15T23:09:09Z

Thank you @ben-manes ! Under write buffers being full I would be surprised if this performed worse than the synchronized map implementation the current default uses. So if fixes in the latest prevent the lost tasks issue from persisting forever, and an upgrade in java version is a "performance boost", then I'm good with making it default and will update the PR with the latest and make caffeine the default (pending other community comments).

ben-manes · 2017-09-15T23:21:45Z

Yes, it would definitely be faster. The full buffer would only mean that the next few writes would be slightly penalized. The maintenance is fast and is used to batch the work, so the buffer is drained letting writes queue up again. The idea being that a cheap buffer write and letting the caller return is faster than blocking on a lock (just like how a write-ahead log speeds db performance).

On the read side, its buffer is lossy and assumes the task will get run properly. It discards new read events in favor of concurrency, since by the power law most frequent would probably get enqueued already. That means only when the write buffer is forced to be processed would the read buffer be, which would have dropped most events. Then the hit rate is diminished since fewer events were recorded & replied against eviction policy, so worse decisions might be made.

So the worst cases are pretty acceptable. They are a tad annoying, but easy to rectify by using an updated JRE.

gianm

I'm in favor of the change (fwiw our distro at Imply already sets caffeine as the cache in the template configs that we ship).

@drcrallen are you wanting to get this in for 0.11?

gianm · 2017-09-16T00:41:20Z


 #### Local Cache

+(*DEPRECATED: Use caffeine instead*)


Suggest this instead.

<div class="note caution"> some nice note </div>

gianm · 2017-09-16T00:44:59Z


+### Caffeine Cache
+
+A highly performant local cache implementation for Druid based on [Caffeine](https://github.com/ben-manes/caffeine). Requires a JRE8u60 or higher


Requires a JRE8u60 or higher

Does it really, or is that just if you use COMMON_FJP?

just for COMMON_FJP

gianm · 2017-09-16T00:45:18Z

+
+Here are the possible values for `druid.cache.cacheExecutorFactory`, which controls how maintenance tasks are run
+
+* `COMMON_FJP` (default) use the common ForkJoinPool. Do NOT use this option unless you are running 8u60 or higher


What's wrong with COMMON_FJP + 8u60? Will you get crashes? Slow behavior? (the docs should say)

And would caffeine with one of the other cacheExecutorFactorys work worse or better than local? (would be great if the docs say this too, if it's something we know)

It is not something that is known for sure. Added a bit of docs about the suggested impact.

gianm · 2017-09-16T00:46:40Z

+        <dependency>
+            <groupId>com.github.ben-manes.caffeine</groupId>
+            <artifactId>caffeine</artifactId>
+            <version>2.3.1</version>


Version should not be specified here, it should come in through dependencyManagement from the main pom.

gianm · 2017-09-16T00:47:39Z

 import com.google.inject.Provider;

-@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl = LocalCacheProvider.class)
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl = CaffeineCacheProvider.class)


Can you please edit the first comment of this PR to indicate that you do want to change the default? This threw me for a loop since it said you weren't going to, and I hadn't read the conversation between you and Ben yet where you changed your mind.

gianm · 2017-09-16T00:49:00Z

-|`query/cache/caffeine/*/requests`|Count of hits or misses|hit + miss|
-|`query/cache/caffeine/*/loadTime`|Length of time caffeine spends loading new values (unused feature)|0|
-|`query/cache/caffeine/*/evictionBytes`|Size in bytes that have been evicted from the cache|Varies, should tune cache `sizeInBytes` so that `sizeInBytes`/`evictionBytes` is approximately the rate of cache churn you desire|
+[Moved](../../configuration/caching.html)


You could add this to _redirects.json instead and get an browser redirect, so people don't have to click the link.

drcrallen · 2017-09-16T02:49:12Z

@gianm It can go out in 0.11.0 or later depending on when it gets merged, I'm not in a hurry since this is a maintenance patch rather than a code change or bug fix.

himanshug · 2017-09-19T19:42:08Z

👍

gianm

LGTM, suggested a minor doc change.

gianm · 2017-09-21T17:11:13Z

 #### Local Cache

+<div class="note caution">
+DEPRECATED: Use caffeine instead


Can you please expand on this a little bit? Imagine a reader that doesn't know why it's deprecated, or when it will be removed, how urgent the switch is, and so on. Maybe something like:

The local cache is deprecated in favor of the Caffeine cache, and may be removed in a future version of Druid. The Caffeine cache affords better performance and control over eviction behavior, and is recommended in any situation where you are using JRE 8u60 or higher.

added

The local cache is deprecated in favor of the Caffeine cache, and may be removed in a future version of Druid. The Caffeine cache affords significantly better performance and control over eviction behavior compared to local cache, and is recommended in any situation where you are using JRE 8u60 or higher.

…feineBase

…tation. (apache#4810) * Move caffeine out of extension. * Remove `JsonTypeName` from the class itself * Fix bad docs * Fix distribution pom * Fix unused import * Make caffeine default * Address code comments * Add more description around the jre version in the readme * Add suggested comments

drcrallen added 2 commits September 15, 2017 15:24

Move caffeine out of extension.

9e06a90

Remove JsonTypeName from the class itself

379821f

drcrallen added the Area - Cache label Sep 15, 2017

drcrallen added 3 commits September 15, 2017 15:34

Fix bad docs

f1e826a

Fix distribution pom

bf790fb

Fix unused import

b372f8e

Make caffeine default

5cfe227

drcrallen changed the title ~~Move caffeine out of extension.~~ Move caffeine out of extension and make it the default cache implementation. Sep 15, 2017

drcrallen closed this Sep 16, 2017

drcrallen reopened this Sep 16, 2017

gianm reviewed Sep 16, 2017

View reviewed changes

gianm added the Release Notes label Sep 16, 2017

leventov and others added 2 commits September 15, 2017 20:54

Merge remote-tracking branch 'upstream/master' into caffeineBase

033f44b

Address code comments

f3c1aa9

Add more description around the jre version in the readme

b793ce3

drcrallen added this to the 0.11.1 milestone Sep 21, 2017

gianm approved these changes Sep 21, 2017

View reviewed changes

drcrallen added 2 commits September 21, 2017 20:52

Add suggested comments

2c00003

Merge remote-tracking branch 'refs/remotes/mmx/caffeineBase' into caf…

586b571

…feineBase

drcrallen merged commit a6470c1 into apache:master Sep 22, 2017

drcrallen deleted the caffeineBase branch September 22, 2017 17:47

jon-wei mentioned this pull request Jan 5, 2018

[WIP] Druid 0.12.0 release notes #5211

Closed


		### Caffeine Cache

		A highly performant local cache implementation for Druid based on [Caffeine](https://github.com/ben-manes/caffeine). Requires a JRE8u60 or higher


		Here are the possible values for `druid.cache.cacheExecutorFactory`, which controls how maintenance tasks are run

		* `COMMON_FJP` (default) use the common ForkJoinPool. Do NOT use this option unless you are running 8u60 or higher

Conversation

drcrallen commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ben-manes commented Sep 15, 2017

Uh oh!

drcrallen commented Sep 15, 2017

Uh oh!

ben-manes commented Sep 15, 2017

Uh oh!

gianm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drcrallen commented Sep 16, 2017

Uh oh!

himanshug commented Sep 19, 2017

Uh oh!

gianm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

drcrallen commented Sep 15, 2017 •

edited

Loading