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

feat: add option to reset ttl on consecutive cache fill #59

Merged
merged 2 commits into from
Mar 13, 2023

Conversation

martensievers
Copy link
Contributor

Have been using this library in a large scale project for caching states for thousands of requests and we noticed that it was missing an option to reset the ttl of an item if you simply use cache.set(key, item) of an already existing key.

Even the option of using bypass doesn't work here as the value already exist in the cache so there is no option other than maybe deleting the item first and then re-adding it which in an asynchronous app could mean that requests will actually miss the cache for a short period of time.

Adding a simple flag on the set() method that will reset the ttl would be a handy option as otherwise an item will still expire despite being re-cached correctly.

@kibertoad
Copy link
Contributor

This is useful and relevant, can it be reviewed?

@avoidwork avoidwork merged commit 3965d67 into avoidwork:master Mar 13, 2023
@kibertoad
Copy link
Contributor

I can create an alternate PR without version bumps and with a test, if that would help.

@avoidwork
Copy link
Owner

thanks!

@avoidwork
Copy link
Owner

@kibertoad there's no conflicts because your PR is clean, so merged and i'll just roll into 10.1.0; ideally we don't change versions in the PRs because they could sit a while.

@kibertoad
Copy link
Contributor

kibertoad commented Mar 13, 2023

@avoidwork Thank you!
Btw, what is the rationale behind not always resetting the TTL on set operation? I wonder if a simpler and more correct change would have been simply always regenerating TTL when SET operation is performed, without any params.

@avoidwork
Copy link
Owner

It was the behavior up to 8.0.0, so this shouldn't have been merged if i understand the goal... you just wanted the 'if' condition removed with 8.0.0 returned?

@avoidwork
Copy link
Owner

I think it's easiest to say <8.0.0 the automatic 'keep ttl' behavior was an edge case for most uses, and people were coding around it with the now private 'has()' and other forms of inspection which can tank performance. Instead of removing the conditional statement I should've introduced a second parameter like this PR.

@kibertoad
Copy link
Contributor

Either one work, since I am aware of the flag existence, I can configure cache to behave in whatever way I need in my own use-cases.
I'm mostly wondering about how confusing it might be to people who don't check the code - I think most developers would assume that when you are setting a new value, you get a new TTL. The fact that it depends on whether or not there is a past value there, seems non-intuitive.
I assume there was a reason why logic changed in 8.0.0, so still having a parameter makes sense; but wouldn't flipping it around to resetting by default be more in line with what most people would expect?
It would require another semver major, of course. If you think it makes sense, I can create a PR.

@avoidwork
Copy link
Owner

Checked my work notes, this was the issue:

  • microservice caches for 5min
  • data refreshes every 30min
  • microservice handles >50req/s so cached item never expired, refresh never triggered the expiration because code didn't have an explicit 'clear()' before the 'set()'.

So 8.0.0 changed to an explicit expiration as a safer behavior.

@avoidwork
Copy link
Owner

Or said differently, a successful & useful implementation of <8.0.0 runs the risk of never expiring items :)

@avoidwork
Copy link
Owner

avoidwork commented Mar 13, 2023

Feel free to open a PR to change that new param to 'true', semver bump to reset behavior with fixed api makes sense to me if other people want it; i think it'd best be done by setting the default from an instance attribute, which defaults to 'true' now.

@avoidwork
Copy link
Owner

avoidwork commented Mar 13, 2023

Wanted to clarify, bypass bypasses has() because get() calls has() before set(); i haven't run 9 or 10 through deoptigate* to see if it's regressing, but the bool skips an op that caused a deopt.

it was never time centric, it was just being misapplied as a signal to perform that 'increment ttl (this is a get)'.

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

Successfully merging this pull request may close these issues.

None yet

3 participants