Skip to content

Support native TTL in entity stores #33

Merged
merged 3 commits into from Sep 18, 2011

2 participants

@aughr
aughr commented Jul 6, 2011

Per #28, I've implemented optionally using an entity store's native TTL support.

aughr added some commits Jul 5, 2011
@aughr aughr Fix status 304 tests.
Rack::Response auto-sets Content-Type, which it then removes in 304s on #finish. To test Rack::Cache, then, we should look at Rack::MockResponse#original_headers.
ac9af58
@aughr aughr Add native-ttl support to entity stores. 298a3ae
@aughr aughr Merge remote-tracking branch 'rtomayko/master' a0a3e92
@rtomayko
Owner

Sorry for the late response.

My only concern is that this will break entity and meta store subclasses in the wild that don't have the ttl argument when the option is enabled. Rails for instance has its own special subclasses that tie into the AS cache framework. I'm going to apply it anyway since the option is disabled by default.

There's another issue with using this approach you should be aware of. If you use a max-age combined with an ETag or Last-Modified header and support validation, the whole body need not be fetched from the backend once expired if the ETag/Last-Modified is still valid. In that case, we simply update the expiration time in the meta store and send back the cached body. By eagerly expiring the body, the backend will need to generate it in full again. Again, I think this is fine as optional behavior.

Thanks!

@rtomayko rtomayko commented on the diff Sep 18, 2011
test/entitystore_test.rb
@@ -187,7 +196,7 @@ describe 'Rack::Cache::EntityStore' do
end
it 'passes options from uri' do
- memcached = Rack::Cache::EntityStore::Dalli.resolve URI.parse("memcached://#{ENV['MEMCACHED']}?show_backtraces=true")
+ memcached = Rack::Cache::EntityStore::MemCached.resolve URI.parse("memcached://#{ENV['MEMCACHED']}?show_backtraces=true")
@rtomayko
Owner
rtomayko added a note Sep 18, 2011

Do you happen to remember why this change was made? It's conflicting on the merge and just seems weird.

@rtomayko
Owner
rtomayko added a note Sep 18, 2011

Nevermind. Looks like it was a sane fix for the context it was in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rtomayko rtomayko added a commit that referenced this pull request Sep 18, 2011
@rtomayko Merge aughr's entity store native ttl support
See #33 for some caveats with this approach.

Conflicts:
	test/entitystore_test.rb
8c666cc
@rtomayko rtomayko merged commit a0a3e92 into rtomayko:master Sep 18, 2011
@rtomayko rtomayko commented on the diff Sep 18, 2011
lib/rack/cache/options.rb
@@ -8,6 +8,8 @@ module Rack::Cache
# below are stored in the Rack Environment as "rack-cache.<option>", where
# <option> is the option name.
module Options
+ extend self
@rtomayko
Owner
rtomayko added a note Sep 18, 2011

Oh hmm. What about this? Is it necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.