Migrate Spray caching as akka-http-caching #213 #986

tomrf1 · 2017-03-21T09:59:59Z

Guided by the discussion here (@ktoso, @ben-manes and @ianclegg) this change migrates the Spray cache and cache directive across to a separate project, akka-http-caching.

This is an almost like-for-like replacement of the spray implementation (including the tests). I've generally left the code the same where possible, but there are some changes:

Whereas spray used ConcurrentLinkedHashMap for LruCache, it now uses caffeine's AsyncLoadingCache.
Removed ascendingKeys from the Cache trait because AsyncLoadingCache doesn't appear to have an equivalent method. I'm also not sure when you'd use it.
Removed implicit ExecutionContext from apply methods because it was only being used for completing the futures, whereas now it simply calls get on the caffeine cache. Does anyone think this will be a problem?
Because AsyncLoadingCache requires a loader on creation, a 'default' loader function must be passed in when creating a LruCache. Calling code can still pass in a separate loading function to apply.
Edit: Changed signature of def remove(key: Any): Option[Future[V]] to def remove(key: Any): Unit, as the value is unlikely to be needed

lightbend-cla-validator · 2017-03-21T10:00:02Z

Hi @tomrf1,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

akka-ci · 2017-03-21T10:00:04Z

Can one of the repo owners verify this patch?

ktoso · 2017-03-21T10:00:53Z

OK TO TEST

ktoso · 2017-03-21T10:01:32Z

NICE! Looking forward to reviewing :-)
We're doing team meeting and planning these few days so sorry for the delay in reviewing that may happen

akka-ci · 2017-03-21T10:12:51Z

Test FAILed.

ben-manes · 2017-03-21T16:31:00Z

This looks really nice!

ascendingKeys(n) is a snapshot from coldest to hottest, which mirrors descendingKeySet that goes hottest to coldest. These were added for Cassandra which wanted to save the cache on shutdown for a warm restart. I'm not sure what they were used for by Spray. The equivalent in Caffeine is in Policy.Eviction, hottest(n) and coldest(n). This can be obtained using cache.synchronous().policy().eviction().

Note that Caffeine uses TinyLFU instead of LRU for the eviction algorithm. Your class names with Lru may give the wrong expectations.

The ExecutionContext could be honored if you use get(key, (key, executor) => future). Instead of using the supplied executor, you could use caller's. Alternatively, you could supply the ExecutionContext at creation time as the cache's executor by setting it in the builder.

It hasn't been clear to me whether introducing an AsyncCache would be useful. I'm glad you found the workaround. If there is a general consensus that it would be a good addition then the API was structures to make it seamless. My concern is that users will be inclined to use getIfPresent and put methods instead of the preferred loading methods, e.g. get(key, func).

As a side note, if there is a desire for variable expiration, then that is a feature that we could collaborate on. The design that I've settled on is a hierarchical timer wheel, as shown in this slide deck (slide 10). This is an O(1) algorithm that would allow entries to have different expiration times, e.g. if caching external http resources based on their expiration header.

ben-manes · 2017-03-21T16:38:06Z

akka-http-caching/src/main/scala/akka/http/caching/LruCache.scala

+
+  def apply(key: Any, genValue: () ⇒ Future[V]): Future[V] = store.get(key, toJavaMappingFunction(genValue)).toScala
+
+  def remove(key: Any): Option[Future[V]] = Option(store.synchronous().asMap().remove(key)).map(Future.successful)


When would a caller require the value on a removal? Its rare for a cache, so the penalty of waiting until the underlying future returns might be avoided.

If this is useful, we could look into supplying an asMap() view on the AsyncLoadingCache.

The spray implementation does not use remove, and I can't think why it would be useful to return the value. I had tried to keep the definitions consistent, but it's tempting to change this to:
def remove(key: Any): Unit

A scan of usages on Github indicate the value is often ignored.

You might argue that this is a simple cache for the common cases, but users with more advanced needs should use Caffeine (or alternatives) directly. That way feature requests are kept at a minimum, as you aren't trying to cater to every edge case.

akka-ci · 2017-03-21T17:19:53Z

Test FAILed.

akka-ci · 2017-03-25T12:07:34Z

Test FAILed.

jonas

LGTM, however it would be nice to also have at least the scaladsl part documented.

For completeness this also needs a Java API but we could deal with that in a separate ticket as long as we mark the API with the Experimental annotation since we cannot guarantee binary compatibility until the Java API is in place.

jonas · 2017-03-25T12:25:22Z

akka-http-caching/build.sbt

@@ -0,0 +1,4 @@
+import akka._
+
+libraryDependencies ++= Seq(


Please define this in the Compile object in project/Dependencies.scala and add to Dependencies.httpCaching

jonas · 2017-03-25T13:57:54Z

akka-http-caching/src/main/scala/akka/http/caching/LfuCache.scala

+   * a non-zero and finite timeToLive and/or timeToIdle is set or not.
+   */
+  def apply[V](
+    defaultLoader:   Any ⇒ Future[V],


Was this formatted with scalariform?

jonas · 2017-03-25T14:15:53Z

akka-http-caching/src/main/scala/akka/http/routing/directives/CachingDirectives.scala

+import akka.http.scaladsl.model.headers.CacheDirectives._
+import akka.http.scaladsl.server.RouteResult.Rejected
+
+trait CachingDirectives {


Each exposed directives should be documented in a separate page in the docs project and listed in the alphabetically and by-trait pages. The spray cache directives docs look like a good start.

Some minimal documentation introducing caching based on
https://github.com/spray/spray/blob/master/docs/documentation/spray-caching/index.rst would also be great. This would also be a good place to make it clear that using the caching directives requires adding an additional dependency.

jonas · 2017-03-25T14:24:45Z

akka-http-caching/src/main/scala/akka/http/routing/directives/CachingDirectives.scala

@@ -0,0 +1,92 @@
+package akka.http.routing.directives


The package for the Scala directives should be akka.http.scaladsl.server.directives

tomrf1 · 2017-03-26T09:42:53Z

@jonas thanks for the feedback, I've pushed some changes plus docs/examples for the caching directives.
I'm not familiar with the experimental annotation, is this specific to akka?

akka-ci · 2017-03-26T09:47:28Z

Test FAILed.

ktoso · 2017-10-31T03:08:23Z

Attempting to get this merged today, thanks for patiently working on this for such a long time!

ktoso · 2017-10-31T06:17:40Z

PLS BUILD

akka-ci · 2017-10-31T06:27:50Z

Test FAILed.

ktoso · 2017-10-31T07:44:09Z

Awesome work @tomrf1!

I believe this is ready to merge and I have some slight follow up changes I'll PR separately. I checked the comments that @jrudolph had and those are addressed as well or I'll do so in the PR I have prepped.

I'll finish figuring out why the build fails with [error] com.lightbend.paradox.markdown.Index$LinkException: Unknown page [low-level-api.md] linked from [java/http/server-side/index.md] etc and merge this PR and do my follow up (prepared already) shortly then.

jonas · 2017-10-31T08:13:00Z

It could be #1371

jonas · 2017-10-31T08:18:16Z

I started porting the doc here: https://github.com/jonas/akka-http/tree/caching-docs

ktoso · 2017-10-31T08:24:28Z

Yeah it's the symlink that did not get created and blows up the PR now https://github.com/akka/akka-http/pull/1371/files#r147919705
It's not really failing the new docs though, but all of the normal dos files (not the caching ones)... very weird.

jonas · 2017-10-31T15:36:44Z

PLS BUILD

akka-ci · 2017-10-31T15:48:14Z

Test FAILed.

raboof · 2017-11-14T14:58:00Z

PLS BUILD

akka-ci · 2017-11-14T15:05:16Z

Test PASSed.

ktoso · 2017-11-15T02:26:47Z

@raboof please notice that I continued work on this over in #1503 and that should be merged (very carefully to keep commits), rather than this PR I think

ktoso

Rather, #1503 should be merged, as I cleaned up some things in it.

akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR that is currently being validated by Jenkins labels Mar 21, 2017

ben-manes reviewed Mar 21, 2017

View reviewed changes

jonas reviewed Mar 25, 2017

View reviewed changes

akka-ci added validating PR that is currently being validated by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) labels Mar 26, 2017

akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR that is currently being validated by Jenkins labels Mar 26, 2017

akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) labels Mar 26, 2017

ktoso self-assigned this Oct 31, 2017

ktoso mentioned this pull request Oct 31, 2017

Migrate Spray caching as akka-http-caching #213 #1503

Closed

ktoso added reviewed Ready to merge from review perspetive, but awaiting some additional action (e.g. order of merges) and removed reviewed Ready to merge from review perspetive, but awaiting some additional action (e.g. order of merges) labels Nov 1, 2017

akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Nov 14, 2017

ktoso suggested changes Nov 15, 2017

View reviewed changes

ktoso closed this Nov 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Spray caching as akka-http-caching #213 #986

Migrate Spray caching as akka-http-caching #213 #986

tomrf1 commented Mar 21, 2017 •

edited

Loading

lightbend-cla-validator commented Mar 21, 2017

akka-ci commented Mar 21, 2017

ktoso commented Mar 21, 2017

ktoso commented Mar 21, 2017

akka-ci commented Mar 21, 2017

ben-manes commented Mar 21, 2017

ben-manes Mar 21, 2017

tomrf1 Mar 21, 2017

ben-manes Mar 21, 2017

akka-ci commented Mar 21, 2017

akka-ci commented Mar 25, 2017

jonas left a comment

jonas Mar 25, 2017

jonas Mar 25, 2017

jonas Mar 25, 2017

jonas Mar 25, 2017

tomrf1 commented Mar 26, 2017

akka-ci commented Mar 26, 2017

ktoso commented Oct 31, 2017

ktoso commented Oct 31, 2017

akka-ci commented Oct 31, 2017

ktoso commented Oct 31, 2017

jonas commented Oct 31, 2017

jonas commented Oct 31, 2017 •

edited

Loading

ktoso commented Oct 31, 2017

jonas commented Oct 31, 2017

akka-ci commented Oct 31, 2017

raboof commented Nov 14, 2017

akka-ci commented Nov 14, 2017

ktoso commented Nov 15, 2017

ktoso left a comment


		def apply(key: Any, genValue: () ⇒ Future[V]): Future[V] = store.get(key, toJavaMappingFunction(genValue)).toScala

		def remove(key: Any): Option[Future[V]] = Option(store.synchronous().asMap().remove(key)).map(Future.successful)

Migrate Spray caching as akka-http-caching #213 #986

Migrate Spray caching as akka-http-caching #213 #986

Conversation

tomrf1 commented Mar 21, 2017 • edited Loading

lightbend-cla-validator commented Mar 21, 2017

akka-ci commented Mar 21, 2017

ktoso commented Mar 21, 2017

ktoso commented Mar 21, 2017

akka-ci commented Mar 21, 2017

ben-manes commented Mar 21, 2017

ben-manes Mar 21, 2017

Choose a reason for hiding this comment

tomrf1 Mar 21, 2017

Choose a reason for hiding this comment

ben-manes Mar 21, 2017

Choose a reason for hiding this comment

akka-ci commented Mar 21, 2017

akka-ci commented Mar 25, 2017

jonas left a comment

Choose a reason for hiding this comment

jonas Mar 25, 2017

Choose a reason for hiding this comment

jonas Mar 25, 2017

Choose a reason for hiding this comment

jonas Mar 25, 2017

Choose a reason for hiding this comment

jonas Mar 25, 2017

Choose a reason for hiding this comment

tomrf1 commented Mar 26, 2017

akka-ci commented Mar 26, 2017

ktoso commented Oct 31, 2017

ktoso commented Oct 31, 2017

akka-ci commented Oct 31, 2017

ktoso commented Oct 31, 2017

jonas commented Oct 31, 2017

jonas commented Oct 31, 2017 • edited Loading

ktoso commented Oct 31, 2017

jonas commented Oct 31, 2017

akka-ci commented Oct 31, 2017

raboof commented Nov 14, 2017

akka-ci commented Nov 14, 2017

ktoso commented Nov 15, 2017

ktoso left a comment

Choose a reason for hiding this comment

tomrf1 commented Mar 21, 2017 •

edited

Loading

jonas commented Oct 31, 2017 •

edited

Loading