Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Replace deprecated `memcache-client` gem with `dalli` in ActiveSupport::Cache::MemCacheStore #6903

Merged
merged 2 commits into from Aug 18, 2012

Conversation

Projects
None yet
9 participants
Owner

guilleiguaran commented Jun 29, 2012

memcache-client was deprecated in favour of dalli in 2010.

The purpose of this PR is offer a better upgrade path for current users of mem_cache_store as was suggested by @jeremy (#6296 (comment))

Owner

rafaelfranca commented Jun 29, 2012

Seems good

Contributor

kennyj commented Jun 29, 2012

👍

👍

Member

steveklabnik commented Jun 29, 2012

👍

@spastorino spastorino commented on the diff Jun 29, 2012

...vesupport/lib/active_support/cache/mem_cache_store.rb
@@ -137,23 +132,17 @@ def write_entry(key, entry, options) # :nodoc:
method = options && options[:unless_exist] ? :add : :set
value = options[:raw] ? entry.value.to_s : entry
expires_in = options[:expires_in].to_i
- if expires_in > 0 && !options[:raw]
- # Set the memcache expire a few minutes in the future to support race condition ttls on read
- expires_in += 5.minutes
- end
@spastorino

spastorino Jun 29, 2012

Owner

Why did you remove this lines?

@guilleiguaran

guilleiguaran Jun 29, 2012

Owner

This code isn't present in Dalli cache store, I'm pretty sure isn't needed if we are using Dalli.

/cc @mperham

@NZKoz

NZKoz Jul 3, 2012

Member

That code's pretty peculiar too, if the user's giving us an expires_in, why would we increment it? If they want to avoid the race conditions they can increment their ttls themselves surely?

@spastorino

spastorino Jul 3, 2012

Owner

@NZKoz yep agree but I'd leave that for another PR :)

@mperham

mperham Jul 3, 2012

Contributor

I think that code can be removed. I was trying to support the race_condition_ttl option but I don't think it's worth it. As koz notes, if the user is savvy enough to want stale reads, they understand that Dalli's expires_in must be incremented to keep the value around for that stale period.

@spastorino

spastorino Jul 3, 2012

Owner

@mperham agree, anyway isn't that code now in dalli?

@mperham

mperham Jul 3, 2012

Contributor

@spastorino It was removed in 2.0. I made an effort to remove edge case code like this that increased the complexity of the code with little benefit.

@spastorino spastorino commented on the diff Jun 29, 2012

...vesupport/lib/active_support/cache/mem_cache_store.rb
@@ -22,21 +22,13 @@ module Cache
# MemCacheStore implements the Strategy::LocalCache strategy which implements
# an in-memory cache inside of a block.
class MemCacheStore < Store
- module Response # :nodoc:
- STORED = "STORED\r\n"
- NOT_STORED = "NOT_STORED\r\n"
- EXISTS = "EXISTS\r\n"
- NOT_FOUND = "NOT_FOUND\r\n"
- DELETED = "DELETED\r\n"
- end
@spastorino

spastorino Jun 29, 2012

Owner

How is this handled now?

@guilleiguaran

guilleiguaran Jun 29, 2012

Owner

Dalli::Client methods return truish/falsy values instead of string responses.

@guilleiguaran

guilleiguaran Jun 29, 2012

Owner

Dalli's increment/decrement are returning integer values or nil (You don't have to use response == Response::NOT_FOUND ? nil : response.to_i anymore)

@ghost ghost assigned guilleiguaran Aug 17, 2012

Owner

jeremy commented Aug 17, 2012

👍 plz rebase & merge

guilleiguaran added some commits Jun 29, 2012

Owner

guilleiguaran commented Aug 18, 2012

@jeremy @spastorino rebased 👍

spastorino added a commit that referenced this pull request Aug 18, 2012

Merge pull request #6903 from guilleiguaran/dalli-memcache-store
Replace deprecated `memcache-client` gem with `dalli` in ActiveSupport::Cache::MemCacheStore

@spastorino spastorino merged commit 9f88521 into rails:master Aug 18, 2012

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