Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Caffeine cache extension #3028

Merged
merged 15 commits into from
Jul 6, 2016
Merged

Caffeine cache extension #3028

merged 15 commits into from
Jul 6, 2016

Conversation

drcrallen
Copy link
Contributor

@drcrallen drcrallen commented May 26, 2016

A new local cache implementation using Caffeine as the cache.

fixes #2411

caffeinehot

We currently use this in production as l1 of a hybrid cache and memcached as the l2. The graph above shows the node mean TopN query time for production servers with and without using this cache in l1 (caffeine = false implies straight-to-memcached).

This extension requires java 8 to run, and therefore has a special profile that activates in the druid.io parent pom only when java8 is detected. This detection will eventually have to be improved to account for java9+

Packaging is accomplished by calling pull-deps a second time during the distribution build IFF the caller is using java8. Otherwise it retains the prior behavior.

In order to get this to work correctly, I had to put the distribution target in its own special rule set that will put it at the END of all other modules, including java8-only ones. If anyone else has a cleaner way of doing this, please let me know.

This is a contribution of https://github.com/metamx/druid-cache-caffeine

@drcrallen drcrallen changed the title Initial commit of caffeine cache Caffeine cache extension May 26, 2016
@@ -0,0 +1,41 @@
Druid Caffeine Cache
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

layout doc_page

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, and in general cleaned up the readme, and moved it to the correct location.

@gianm
Copy link
Contributor

gianm commented May 26, 2016

@drcrallen do you have any comparison of caffeine vs the existing local cache?

Also what does this part mean?

I have not yet figured out a good way to get the distribution to switch for jdk7 vs 8.

If that is just about the tarball, IMO it's ok for us to just have one distribution, with most modules targeting 1.7 but this module targeting 1.8. If the pom achieves that then I think it is good the way it is.

@drcrallen
Copy link
Contributor Author

@gianm I didn't touch the distribution packaging extension. I think your suggestion is reasonable but would like to tackle distribution of this extension in a separate PR with appropriate community discussion and feedback.

@drcrallen
Copy link
Contributor Author

For local cache I have no comparison because its lock contention falls flat on its face in our production cluster. See #1836 and related.

@drcrallen
Copy link
Contributor Author

Also, this fixes #2411 (will add to master comment when I'm not on mobile)

@gianm
Copy link
Contributor

gianm commented May 26, 2016

@gianm I didn't touch the distribution packaging extension. I think your suggestion is reasonable but would like to tackle distribution of this extension in a separate PR with appropriate community discussion and feedback.

As long as we can get this figured out before the next release, that sounds fine. It's marked extensions-core (which SGTM) but as a core extension it should be in the tarball.

For local cache I have no comparison because its lock contention falls flat on its face in our production cluster. See #1836 and related.

Would it be possible to do synthetic benchmarks? I'm not gonna insist but I'm just curious and others might be too.


### Jars

For the sake of sanity if you are manually making an extension directory, release requires the following jars:
Copy link
Contributor

@gianm gianm May 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "for the sake of sanity" mean in this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can edit it to be clearer. I was trying to indicate this extension should be packaged with two jars (as of 0.9.0)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO before this is in a release we should figure out how to make it part of the tarball, meaning this section and the earlier "How to use" won't be necessary.


# Versioning

The versioning works like this: `druid_version.patch_set`. Such that `druid_version` is the version of druid the extension was compiled against, and `patch_set` is the "version" of the extension.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't do this with any other extensions. Any reason to have this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, no, need to remove.

@gianm
Copy link
Contributor

gianm commented May 26, 2016

👍 on the code, although would like to figure out the packaging before release if possible. For a core extension "it's in the tarball, just update loadList" is a better story than "build it yourself or use pull-deps".

@drcrallen drcrallen added this to the 0.9.2 milestone May 26, 2016
@drcrallen
Copy link
Contributor Author

@gianm I'll tag it for 0.9.2 for now. Hopefully I can bang around the distribution stuff a bit after 0.9.1 is qualified.

@drcrallen
Copy link
Contributor Author

Anyone who REALLY REALLY wants to use this NOW can use https://github.com/metamx/druid-cache-caffeine .

One of the advantages of being in druid proper is the built-in pacakging, so I'm ok if it is desired to be solved before merging.

@ben-manes
Copy link

@drcrallen Can you try upgrading to 2.3.1? I assume you haven't looked into the failure, but perhaps it was due to ben-manes/caffeine#90.

@drcrallen
Copy link
Contributor Author

@ben-manes thanks for the info. Yes I'll update for this PR

For our prod servers we're using single-threaded executor since the moral of the story in general is "don't use java common FJP"

@ben-manes
Copy link

In that case it was my fault. The bug was caused by all of the executor's task writing back to the cache. This would happen on the completion of a future (to assign a weight) or refresh to insert the new value. Previously the write buffer was unbounded, but when we switched to a bounded buffer I forgot to handle the case of all executor tasks performing a write-back. That meant none could proceed as they waited for an asynchronous task queued up, but with no threads available. This was surprising to have hit outside of a stress test given the available buffer slack, but it happened to a user.

It was an easy fix and also improved eviction throughput (2.5M/s => 4M/s). I don't think you'd hit this bug given your usage patterns, though.

@drcrallen
Copy link
Contributor Author

Failed due to ben-manes/caffeine#93

Druid Caffeine Cache
--------------------

A highly performant local cache implementation for Druid based on [Caffeine](https://github.com/ben-manes/caffeine). Requires a JRE with a fix for https://bugs.openjdk.java.net/browse/JDK-8078490
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a note caution and just mention this extension only works for java 8?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -28,7 +28,8 @@ Core extensions are maintained by Druid committers.
|druid-kafka-eight|Kafka ingest firehose (high level consumer).|[link](../development/extensions-core/kafka-eight-firehose.html)|
|druid-kafka-extraction-namespace|Kafka-based namespaced lookup. Requires namespace lookup extension.|[link](../development/extensions-core/kafka-extraction-namespace.html)|
|druid-lookups-cached-global|A module for [lookups](../querying/lookups.html) providing a jvm-global eager caching for lookups. It provides JDBC and URI implementations for fetching lookup data.|[link](../development/extensions-core/lookups-cached-global.html)|
|druid-s3-extensions|Interfacing with data in AWS S3, and using S3 as deep storage.|[link](../development/extensions-core/s3.html)|
|druid-s3-extensions|Interfacing with data in AWS S3, and using S3 as deep storage.|[link](../development/extensions-core/caffeine-cache.html)|
|druid-caffeine-cache|A local cache implementation backed by Caffeine.|[link](../development/extensions-core/s3.html)|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit, but can we sort these lexicographically?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, fixed

@fjy
Copy link
Contributor

fjy commented Jul 1, 2016

👍

@drcrallen
Copy link
Contributor Author

@gianm I did some packaging magic, please check again

@gianm
Copy link
Contributor

gianm commented Jul 6, 2016

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a caffeine cache extension
4 participants