Fixes #243 honoring mutable cache ttl-ness in getOrElseUpdate and hit #244
Conversation
| v | ||
| } | ||
| } | ||
| // hit(k) should not update the the underlying entry since we want to honor the ttl and |
There was a problem hiding this comment.
So, the intent of hit is that it is refreshing the cache. If you don't want to refresh the cache and keep the key hot, you use get.
I think you want a slightly different behavior actually. You want a TTL that expires from the first entry, no matter what.
Should we make a different class to address this, or is the current use case totally unreasonable?
There was a problem hiding this comment.
This possibly does look like an error though(just a different thing than this is aiming to fix...), in that hit should use get on the cache followed by a += so that we don't hit something that has already expired. Effectively the value shouldn't be in the backing cache anymore.
There was a problem hiding this comment.
The way I discovered the "issue" was by using a CachedReadableStore with a MutableTTLCache being the cache implementation of the store. I am basically using it to store metadata about some object that doesn't change frequently, but is needed on a frequent basis. I don't want to call out to my underlying store more than necessary, and working on slightly stale data is acceptable. In this case, CachedReadableStore calls getOrElseUpdate which calls hit() on the underlying cache, not get(). This makes perfect sense for something like an LRUCache, so I didn't want to change the behavior or getOrElseUpdate as that would be more far reaching than necessary, and probably not even correct.
When I made the changes I thought this was just a bug in how ttl-ness was being interpreted. Now I can see situations where a "soft-ttl" is desirable, like storing a user's session based on activity. However, having a "hard-ttl" is also desirable if you don't want the information in the cache longer than the specified ttl, period. In any event, bypassing the call to get() in the hit() function also bypasses any ttl-ness set on the entry, and I think needs to be addressed regardless, as @ianoc alluded to.
However, when dealing with a strict ttl, I do not think a call to hit() should call += and effectively reset the clock on the ttl. Perhaps adding a flag to the constructor would be the best way to achieve the desired behavior? Something like
MutableTTLCache(..., strictTTL: Boolean = false). We would obviously default to false which would maintain any backward compatibility.
|
Aaron Schwager seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
I'd like to upvote this PR. I just had this soft-TTL issue bite me a bit because I didn't understand how it works. I'd like to see an option to make the behavior controllable at cache creation (either hard or soft TTL-ness). |
No description provided.