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

Re-add support for memory_store #161

Merged
merged 3 commits into from
Jun 11, 2014
Merged

Re-add support for memory_store #161

merged 3 commits into from
Jun 11, 2014

Conversation

fbogsany
Copy link
Member

@fbogsany fbogsany commented Jun 9, 2014

PR #154 removed support for memory_store, due to the requirement for cas/cas_multi.

This adds a MemoryFetcher ands uses it instead of CacheFetcher if the cache_backend doesn't respond to cas and cas_multi.

Fixes #160

@arthurnn @dylanahsmith for review /cc @camilo

@dylanahsmith
Copy link
Contributor

Could you also add a test for fetch_multi when using the MemoryStore?


def fetch_multi(keys, &block)
results = @cache_backend.read_multi(keys)
hit_keys = results.reject {|key, value| value == nil }.keys
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is necessary. I think it omits keys from the hash if they aren't found in the cache.

@dylanahsmith
Copy link
Contributor

LGTM other than my 2 comments.

@RobWu
Copy link

RobWu commented Jun 10, 2014

Get the same issue with dalli gem

/gems/identity_cache-0.2.0/lib/identity_cache/cache_fetcher.rb:49:in `cas_multi': undefined method `cas_multi' for #<ActiveSupport::Cache::DalliStore:0x007fb15c667000> (NoMethodError)

Would be great if it still would be possible to use alternativ memcached clients.

@camilo
Copy link
Contributor

camilo commented Jun 10, 2014

@RobWu we are moving towards CAS over SET to avoid race conditions that can potentially return wrong data, is there a version of Dalli that supports CAS?

@RobWu
Copy link

RobWu commented Jun 10, 2014

There is cas but can't find multi_cas

http://www.rubydoc.info/github/mperham/dalli/Dalli/Client#cas-instance_method

@camilo
Copy link
Contributor

camilo commented Jun 10, 2014

@RobWu yeah. @fbogsany had to add multi cas support to memcached for this to work, so dalli would need the same, we probably don't have the time now to add it to dalli, but that is about all that would need to happen, for it to work with master.

@RobWu
Copy link

RobWu commented Jun 10, 2014

@RobWu
Copy link

RobWu commented Jun 10, 2014

Will look into it tomorrow..
Use version 0.1.0 for now.

@arthurnn
Copy link
Contributor

I am not sure if I like this approach.
What we are trying to solve here should be, make IDC work with other adapters(which include the memory one). So IMO, we should have a fallback backend which still uses sets, for people that dont use memcached gem, and not only create a fallback for memory stores.

If thats too hard, we maybe should drop support to the other adapters, and raise an error if the adapter is not memcached_store.

@fbogsany
Copy link
Member Author

@arthurnn is that just a critique of the MemoryFetcher name? Other than the name, I think this PR does what you suggest.

@arthurnn
Copy link
Contributor

@fbogsany sorry, I should've been more clear about it.. Yep.. the name MemoryFetcher is not ideal. But thats my point only.. the rest looks good.

@fbogsany
Copy link
Member Author

@arthurnn groovy - does FallbackFetcher work? Or LegacyCacheFetcher? Naming is hard...

@arthurnn
Copy link
Contributor

I like FallbackFetcher better. but both would work for me.. ❤️

@fbogsany
Copy link
Member Author

FallbackFetcher it is.

@camilo
Copy link
Contributor

camilo commented Jun 11, 2014

🐑

fbogsany added a commit that referenced this pull request Jun 11, 2014
Re-add support for memory_store
@fbogsany fbogsany merged commit b19a6b3 into master Jun 11, 2014
@fbogsany fbogsany deleted the memory_fetcher branch June 11, 2014 14:13
@arthurnn
Copy link
Contributor

❤️ ❤️ ❤️

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.

fetch_multi fails when using memory_store
5 participants