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

Cache handles stale values #22

Closed
wants to merge 6 commits into from
Closed

Cache handles stale values #22

wants to merge 6 commits into from

Conversation

bargulg
Copy link

@bargulg bargulg commented Jan 17, 2016

This is needed for any reasonable cache that can set expire time on keys and remove them automatically. For example redis

@wtfrank
Copy link
Contributor

wtfrank commented Jun 8, 2016

Isn't it a bit weird to put the expiry time as an offset from the current time, rather than an absolute time: e.g. "ex + time.time()"? Cos small processing delays will push the calculated expiry time slightly into the future, which wouldn't happen with an absolute time.

@bargulg
Copy link
Author

bargulg commented Jun 8, 2016

well, from the API we get cache time as an offset, and i don't see any real reason to translate it, since it would make things more complicated and those processing delays are very small... it would be different if cache time was shorter, but for something like 5 minutes, it doesn't matter

@jonobrien
Copy link
Contributor

has anyone tested this pr?

@wtfrank
Copy link
Contributor

wtfrank commented Jul 25, 2016

Do you think it would be useful to expose the expiry time (and the original object creation/cache time) to the user somehow? For example, if I want to track a character's location in a wormhole mapping application then I would want to fire off a new location request as soon as the previous one had expired.

@jonobrien
Copy link
Contributor

That use-case sounds useful definitely, might need more testing to see what is possible.

@bargulg
Copy link
Author

bargulg commented Jul 27, 2016

i've been using my own version with this patch for half a year now, and it works and it makes it a lot easier to create real caching mechanisms... but now there's been some change adding memcached to the code of of pycrest so that would need to be adapted to this patch, too... and i don't care enough about pycrest to fix it anymore, because this has been waiting here for half a year

@hkraal
Copy link
Contributor

hkraal commented Jul 27, 2016

@bargulg it's a shame it hasn't been picked up back in the day and your efforts went to waste due changes along the way.

That being said a feature like this will find it's way into the next pycrest release if it's up to me. My concept ( #37 ) results in an dictionary being stored as a whole in the cache. In that dictionary there will be an "expire_in" and "expire_at" property on which the caching can take his course of action using the relative or absolute time.
It's intended to fix a few other things as well like providing CREST endpoint version information which seems to be a popular request.

I've opened #38 to track this specific feature so we can, if you agree, close this PR for now.

@hkraal hkraal closed this Mar 12, 2017
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.

4 participants