Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion doc/configuration.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -124,4 +124,13 @@ which will act as the cache key generator:
end

For more options see the [Rack::Request documentation](http://rack.rubyforge.org/doc/classes/Rack/Request.html)


### `use_native_ttl`

Passes on the expiration timestamp to the cache store. This may be necessary

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like that support it, like memcached and redis could be good to not make ppl think heap/file support it ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix.

with some stores to keep them from filling up, e.g. if using a Redis backend and

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you wrote your own redis backend ? ... want to link that in the readme ?
... FYI I kind of want to get rid of all these silly backends and just support a common read/write interface and be one with it ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you wrote your own redis backend

I didn't. This PR is mainly about making Rack::Cache work correctly with the redis-store/redis-rack-cache gem.

want to link that in the readme

Sure thing.

FYI I kind of want to get rid of all these silly backends and just support a common read/write interface and be one with it

Not sure what you mean? To my understanding Rack::Cache already has pluggable backends, as long as they're namespaced correctly, respond to a .resolve(uri) factory, and have instances that quack #exists?, #read, and #write?

(edit: markdown)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is entity and meta store ... and there should be no reason
redis-rack-cache is needed, it should just be store: Redis.new or Rails.cache and done :D
But that's for another PR ... just FYI since I thought you built your own
backend and had input to that idea.

On Tue, Feb 2, 2016 at 12:32 AM, Julien Letessier notifications@github.com
wrote:

In doc/configuration.markdown
#117 (comment):

@@ -124,4 +124,13 @@ which will act as the cache key generator:
end

For more options see the Rack::Request documentation

+### use_native_ttl
+
+Passes on the expiration timestamp to the cache store. This may be necessary
+with some stores to keep them from filling up, e.g. if using a Redis backend and

you wrote your own redis backend
I didn't. This PR is mainly about making Rack::Cache work correctly with
the redis-store/redis-rack-cache
https://github.com/redis-store/redis-rack-cache gem.

want to link that in the readme
Sure thing.

FYI I kind of want to get rid of all these silly backends and just support
a common read/write interface and be one with it
Not sure what you mean? To my understanding Rack::Cache already has
pluggable backends, as long as they're namespaced correctly, respond to a
.resolve(uri) factory, and have instances that quack #exists?, #read, and
#write?


Reply to this email directly or view it on GitHub
https://github.com/rtomayko/rack-cache/pull/117/files#r51537743.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should just be store: Redis.new or Rails.cache and done :D

I'd agree with you... mostly. We've been bitten by the fact the current Rack::Cache doesn't support dependency-injecting one's store (except with the kludgy hack for Memcache), because that causes 2 extra connections per process to the backend to be created (Redis, in our case). Not ideal scaling wise.

But, most users (at small scale) probably appreciate the simplicity of the current interface? As in, provide a URL and be done with it — no need to instantiate your own store.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(also, going off-topic. Sorry. I'll get back to fixing this PR.)

the `volatile-ttl` expiration policy.

If using `memcached`, it will speed up misses slightly as the middleware won't
need to fetch metadata and check timestamps.

20 changes: 12 additions & 8 deletions lib/rack/cache/meta_store.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, entries
    
  •  if 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.

write key, entries, response.ttl
else
write key, entries
end
key
end

Expand Down Expand Up @@ -155,7 +159,7 @@ def read(key)
# Store an Array of request/response pairs for the given key. Concrete
# implementations should not attempt to filter or concatenate the
# list in any way.
def write(key, negotiations)
def write(key, negotiations, ttl = nil)
raise NotImplementedError
end

Expand Down Expand Up @@ -188,7 +192,7 @@ def read(key)
end
end

def write(key, entries)
def write(key, entries, ttl = nil)
@hash[key] = Marshal.dump(entries)
end

Expand Down Expand Up @@ -226,7 +230,7 @@ def read(key)
[]
end

def write(key, entries)
def write(key, entries, ttl = nil)
tries = 0
begin
path = key_path(key)
Expand Down Expand Up @@ -325,9 +329,9 @@ def read(key)
cache.get(key) || []
end

def write(key, entries)
def write(key, entries, ttl = 0)
key = hexdigest(key)
cache.set(key, entries)
cache.set(key, entries, ttl)
end

def purge(key)
Expand Down Expand Up @@ -359,9 +363,9 @@ def read(key)
[]
end

def write(key, entries)
def write(key, entries, ttl = 0)
key = hexdigest(key)
cache.set(key, entries)
cache.set(key, entries, ttl)
end

def purge(key)
Expand Down
6 changes: 6 additions & 0 deletions test/meta_store_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,12 @@ def purge(*args); nil end

@store.read(key).length.must_equal 2
end

it 'takes a ttl parameter for #write' do
@store.write('/test', [[{},{}],[{},{}]], 0)
tuples = @store.read('/test')
tuples.must_equal [ [{},{}], [{},{}] ]
end
end
end
end
Expand Down