This project is archived and is in readonly mode.
Increment/decrement semantics vary between Rails.cache stores
This issue was discussed in more detail on the Rails Core mailing list. http://groups.google.com/group/r...
Essentially, each cache store has different semantics for incrementing & decrementing counters in the cache. The desired semantics can be expressed with the following test cases.
def test_increment
@cache.write('foo', 1)
assert_equal 1, @cache.read('foo')
assert_equal 2, @cache.increment('foo')
assert_equal 2, @cache.read('foo')
assert_equal 3, @cache.increment('foo')
assert_equal 3, @cache.read('foo')
end
def test_decrement
@cache.write('foo', 3)
assert_equal 3, @cache.read('foo')
assert_equal 2, @cache.decrement('foo')
assert_equal 2, @cache.read('foo')
assert_equal 1, @cache.decrement('foo')
assert_equal 1, @cache.read('foo')
end
Unfortunately, due to MemCache's requirement that counter data be stored in raw format, that syntax needs to be modified to following if the MemCache store is to be supported. This style, while ugly, will still work across all stores.
def test_increment
@cache.write('foo', 1, :raw => true)
assert_equal 1, @cache.read('foo', :raw => true).to_i
assert_equal 2, @cache.increment('foo')
assert_equal 2, @cache.read('foo', :raw => true).to_i
assert_equal 3, @cache.increment('foo')
assert_equal 3, @cache.read('foo', :raw => true).to_i
end
def test_decrement
@cache.write('foo', 3, :raw => true)
assert_equal 3, @cache.read('foo', :raw => true).to_i
assert_equal 2, @cache.decrement('foo')
assert_equal 2, @cache.read('foo', :raw => true).to_i
assert_equal 1, @cache.decrement('foo')
assert_equal 1, @cache.read('foo', :raw => true).to_i
end
-
Michael Koziarski- Assigned user set to Michael Koziarski
Could you give us a version of this patch which doesn't touch the memcache stuff, but just tidies up the semantics of the other stores?
Then we'll need to get the memcache stuff fixed upstream, it's also broken in the MemCache gem from evan weaver.
-
Doug BarthMichael, I will work on that change, but I'm a bit confused as to why the current patch can't be applied as is. I didn't make any modifications to the vendor-ized memcache-client library included with ActiveSupport. All changes were in the ActiveSupport::Cache::Store subclasses, which, as far as I know, are only maintained in the ActiveSupport codebase.
-
Michael Koziarski
I'm pretty confident that we can and will apply it, I just want to have that patch seperately so we can use it when talking with the various memcache driver maintainers. e.g. we can say "This is the problem, does this fix look right" without having a bunch of other changes in there to confuse them
-
Doug Barth
Thanks for the clarification Michael. In that case, I will split this patch into two. One to fix all the cache stores minus MemCacheStore & CompressedMemCache and the second will fix the remaining stores.
On a related note, do you know what process needs to be spun up to successfully use the DRbStore?
On Oct 9, 2008, at 9:47 AM, Lighthouse wrote:
-
-
-
Michael Koziarski
I have no idea, I believe it's just a drb script which exposes a hash...
No one uses it and it should probably be removed post 2.2.
-
Michael Koziarski
The raw fix doesn't seem to help matters, as you can't read the value again once it's been created due to marshalling errors
Seems that (ironically) we can't support incr/decr for memcache at all?
-
Doug Barth
Michael, do you have a test case that exposes the error you're seeing? Everything worked fine for me so long as you also read the value with :raw => true.
What are your thoughts on creating separate methods for reading/ writing counter values? That would allow each store to hide it's own peculiarities for counters behind those methods.
-
Michael Koziarski
Yeah, sorry, I messed up the testing. Never test from the railway station :)
However could you clarify one thing. This line is to work around the memcache-client bug right:
+ value = value.to_s if raw?(options) -
Michael Koziarski
- Milestone cleared.
-
Doug Barth
Yep.
-
Michael Koziarski
OK, I've got this applied locally, the only catch is it shouldn't test the memcache store tests when there's no memcache connection available, but currently it does. So if there's no memcached process running, I get errors.
Can you address this in another patch, then I think we're good to go.
Thanks for the good work!
-
-
