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

Add cache_store adapter for caching via ActiveSupport::CacheStore #265

Merged
merged 2 commits into from Jun 11, 2017

Conversation

Projects
None yet
2 participants
@AlexWheeler
Collaborator

AlexWheeler commented Jun 8, 2017

#263

  • Helpful adapter for Rails users that want to use their configured cache store cache.

http://api.rubyonrails.org/classes/ActiveSupport/Cache/Store.html

  • can pass options as described in docs above e.g. :expires_in to Adapters::CacheStore constructor to use this for all flipper keys. Otherwise it will default to whatever the user has set on the cache via its constructor.

From docs ^ Some implementations may not support all methods beyond the basic cache methods of fetch, write, read, exist?, and delete. Sinc we're using fetch_all we might want to check if the @cache responds to fetch_all and if not then call our own fetch_all that loops with fetch. Thoughts?

@AlexWheeler AlexWheeler force-pushed the active-support-cache-store branch from af4fc96 to 522b60a Jun 8, 2017

Add cache_store adapter for caching via ActiveSupport::CacheStore
* Helpful adapter for Rails users that want to use their configured
cache.
@jnunemaker

This comment has been minimized.

Owner

jnunemaker commented Jun 8, 2017

I think because not all methods support the same options, I would be inclined to be explicit about the options we support and where we support them. TTL/expires seems like the most sensible one to support out of the gate. Any other obvious ones to support?

From docs ^ Some implementations may not support all methods beyond the basic cache methods of fetch, write, read, exist?, and delete. Sinc we're using fetch_all we might want to check if the @cache responds to fetch_all and if not then call our own fetch_all that loops with fetch. Thoughts?

I don't understand this part. I don't see fetch_all in the PR.

@AlexWheeler

This comment has been minimized.

Collaborator

AlexWheeler commented Jun 8, 2017

I would be inclined to be explicit about the options we support and where we support them
That totally makes sense to me. I'll check to see if any others make sense too.

I don't see fetch_all in the PR.
You are correct, that's a typo on my part, I meant read_multi which we're using in get_multi.

@AlexWheeler

This comment has been minimized.

Collaborator

AlexWheeler commented Jun 8, 2017

It looks like no need for the custom read_multi nonsense I mentioned above. Cache implements this and caches (such as MemoryStore) can implement it themselves if they want to optimize it. So we're all good there, if the cache doesn't implement it it will just fall back to this default implementation via the looping read().

https://github.com/rails/rails/blob/master/activesupport/lib/active_support/cache.rb#L353

@AlexWheeler AlexWheeler force-pushed the active-support-cache-store branch 2 times, most recently from b2f56af to 0087b4e Jun 9, 2017

Accept expires_in CacheStore contructor over options hash
* Before we accepted all options passed them onto the cache methods, but
this allows us to be more specific

* use a write_options hash since we don't want to override the default
expires_in if its already been set on the cache instance

@AlexWheeler AlexWheeler force-pushed the active-support-cache-store branch from 0087b4e to 17ffbec Jun 9, 2017

@name = :cache_store
@cache = cache
@write_options = {}
@write_options.merge!(expires_in: expires_in) if expires_in

This comment has been minimized.

@AlexWheeler

AlexWheeler Jun 9, 2017

Collaborator

ActiveSupport::Cache will use ttl set on the @cache getting passed into this initializer, but allows you to set a ttl for a given key when using fetch/write. Doing it this way we won't clobber the already set ttl, but still allow users to use a different ttl for flipper queries.

Its also possible that users won't even want to use a different ttl here and will always want to use whatever they're cache store is using when they initialize it?

@AlexWheeler

This comment has been minimized.

Collaborator

AlexWheeler commented Jun 9, 2017

The other options described here are:
:force
:compress
:race_condition_ttl

and I don't think it makes sense to let users set these at the moment just for flipper stuff.

:namespace can be set as well, but we don't want to change the namespace they've already set on their cache store! Instead flipper keys will have their own namespace tacked on the same way the other cache adapters work.

@AlexWheeler AlexWheeler closed this Jun 9, 2017

@AlexWheeler AlexWheeler reopened this Jun 9, 2017

@jnunemaker

Looks good! I agree on waiting to add the other keys.

@jnunemaker jnunemaker merged commit eac0664 into master Jun 11, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jnunemaker jnunemaker deleted the active-support-cache-store branch Jun 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment