Metastore TTL support #117
Conversation
|
can you rebase this so I can merge / see what is new ? |
|
@grosser: sure thing. Bear with me as master has diverged a bit ^__^ |
| @@ -81,7 +81,11 @@ def store(request, response, entity_store) | |||
| headers.delete 'Age' | |||
|
|
|||
| entries.unshift [stored_env, headers] | |||
| write key, entries | |||
| if request.env['rack-cache.use_native_ttl'] && response.fresh? | |||
There was a problem hiding this comment.
would this be better as an option when creating the store ?
this is supposed to be passed in from the outside ... or via another middleware ?
There was a problem hiding this comment.
would this be better as an option when creating the store ?
It might, but I was trying to keep this PR about the bug fix — the rack-cache.use_native_ttl env field was already present in other parts of the code (writing to the entity store, just above in this same file).
It just wasn't used properly with metastore writes, and wasn't documented.
I'm happy to make this a store option, but as that would be a breaking change (some users have code that reliad on use_native_ttl), maybe it'd be a better fit in a further PR?
There was a problem hiding this comment.
ah, did not know that, sounds good then!
On Tue, Feb 2, 2016 at 12:38 AM, Julien Letessier notifications@github.com
wrote:
In lib/rack/cache/meta_store.rb
#117 (comment):@@ -81,7 +81,11 @@ def store(request, response, entity_store)
headers.delete 'Age'entries.unshift [stored_env, headers]
write key, entriesif request.env['rack-cache.use_native_ttl'] && response.fresh?would this be better as an option when creating the store ?
It might, but I was trying to keep this PR about the bug fix — the
rack-cache.use_native_ttl env field was already present in other parts of
the code (writing to the entity store
https://github.com/rtomayko/rack-cache/blob/master/lib/rack/cache/meta_store.rb#L60,
just above in this same file).
It just wasn't used properly with metastore writes, and wasn't documented.I'm happy to make this a store option, but as that would be a breaking
change (some users have code that reliad on use_native_ttl), maybe it'd
be a better fit in a further PR?—
Reply to this email directly or view it on GitHub
https://github.com/rtomayko/rack-cache/pull/117/files#r51538218.
|
how about a test that uses |
|
|
||
| ### `use_native_ttl` | ||
|
|
||
| Passes on the expiration timestamp to the cache store. This may be necessary |
There was a problem hiding this comment.
something like that support it, like memcached and redis could be good to not make ppl think heap/file support it ...
Done. Not the most elegant but I don't see a better way until we switch the whole thing do injected backend stores (which would be great for testing!) |
mezis
changed the title
|
this will blow up custom stores that do not support ttls ... but only if you also have native_ttl enabled ... so hopefully not too bad ... I like that it makes it more consistent with the entity stores that already write with a ttl ... |
|
mocks make the test a bit easier to understand 2d66181 |
This contains #92 and #93, adds travis build configuration, adds missing docs for
use_native_ttl, and cleans up the Rakefile.@rtomayko if you're too busy I'm happy to help maintain this, just let me know.