-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Don't cache elements for 1 second, makes no sense #11516
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,7 +31,7 @@ private[caffeine] class DefaultCaffeineExpiry extends Expiry[String, ExpirableCa | |
|
||
private def calculateExpirationTime(durationMaybe: Option[Duration]): Long = { | ||
durationMaybe match { | ||
case Some(duration) if duration.isFinite && duration.lteq(0.second) => 1.second.toNanos | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The 1 second for caffeine was introduced in #9615 - there is no comment why this was done, so my guess would be the author just copied the ehcache behaviour... Update: comment was added. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So in ben-manes/caffeine#803 it turns out I was right and returning 0 will therefore always end up in a cache miss for the next read from the cache from the same key, so it's like not caching at all. |
||
case Some(duration) if duration.isFinite && duration.lteq(0.second) => 0.seconds.toNanos | ||
case Some(duration) if duration.isFinite => duration.toNanos | ||
case _ => Long.MaxValue | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -188,19 +188,24 @@ private[play] case class EhCacheExistsException(msg: String, cause: Throwable) e | |
class SyncEhCacheApi @Inject() (private[ehcache] val cache: Ehcache) extends SyncCacheApi { | ||
override def set(key: String, value: Any, expiration: Duration): Unit = { | ||
val element = new Element(key, value) | ||
var doCache = true | ||
expiration match { | ||
case infinite: Duration.Infinite => element.setEternal(true) | ||
case finite: FiniteDuration => | ||
val seconds = finite.toSeconds | ||
if (seconds <= 0) { | ||
element.setTimeToLive(1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First, Play's ehcache implementation introduced that behaviour first, see https://github.com/playframework/playframework/pull/5923/files#r1005645691 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I mentioned in my reply on that PR, we should consider cases where the duration is a positive value less than 1 second. If the duration is actually zero or less I agree it's reasonable behavior not to cache at all. |
||
// We don't even put the element in the cache, why should we? | ||
// Obviously someone wants to put something in the cache for 0 (or less) seconds... | ||
doCache = false | ||
} else if (seconds > Int.MaxValue) { | ||
element.setTimeToLive(Int.MaxValue) | ||
} else { | ||
element.setTimeToLive(seconds.toInt) | ||
} | ||
} | ||
cache.put(element) | ||
if (doCache) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there is an existing value in the cache, I think you want to remove the element from the cache rather than just doing nothing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that is something to consider, thanks! |
||
cache.put(element) | ||
} | ||
Done | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if condition was done just to be on par with the ehcache implementation. Actually belows change in
calculateExpirationTime
from 1 to 0 should already be enough.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkurz I didn't understand.
For me, if some uses the
set(Key: String, value: Any, expiration: Duration)
method it won't be saved intosyncCache
. So, it seems that this method wouldn't work...IMHO doesn't matter if your pass 0+ seconds, must work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does work for 1 and more seconds, but an element will not be cached if 0 seconds or lower.
The condition means
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a negation of the if below
case Some(duration) if duration.isFinite && duration.lteq(0.second) =>...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what I've said seems to be true.
If someone uses
0 seconds
it wouldn't be saved, right?Why wouldn't we set it into the cache as a value without expiration when is defined as 0 secs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is the current behavour in Play 2.8. as well. OK, not really, currently, if you pass 0 that means cache it just for 1 second.... this is what I am changing right now because it does not make sense in my opinion...
Maybe this code explains the current behavour best:
playframework/cache/play-ehcache/src/main/scala/play/api/cache/ehcache/EhCacheApi.scala
Lines 190 to 204 in ccf99ba
Because someone, long long time ago, decided that when passing 0 as expiration it should mean "do not cache" (or like: cache for 1 second) in Play. Be aware, that different libraries use different approaches. So for ehcache if you pass 0 that means cache it forever, however, in caffeine if you return 0 from
expireAfterCreate
it means do not cache it at all (see this coment "...How well couldexpireAfter(expiry)
fit your use-case? That returns a duration, which may be zero to indicate immediate expiration...")So different libraries use different approaches. And Play choose to say 0 expiration = no caching. I didn't decided that.
So my changes here do not change anything actually, I just do not cache it for 1 second but do not cache it at all anymore because it's senseless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felipebonezi Please let me know if you don't understand what I am saying. In my opinion my code does not really change the behaviour of the cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, I do understand now.
Thanks for being patient with me, hehehehe. 🤣