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

Add get-with-cas ('gets') / set-with-CAS functionality #398

Merged
merged 1 commit into from Dec 19, 2013

Conversation

Projects
None yet
6 participants
@philipmw
Copy link
Contributor

commented Oct 21, 2013

This patch allows safe updates of values in a scenario with multiple writers.

ttl ||= @options[:expires_in].to_i
(value, cas) = perform(:cas, key)
value = (!value || value == 'Not found') ? nil : value
value.nil? ? nil : ValueWithCas.new(value, cas)

This comment has been minimized.

Copy link
@mperham

mperham Oct 21, 2013

Collaborator

I would rather this just return [value, cas]

This comment has been minimized.

Copy link
@philipmw

philipmw Oct 21, 2013

Author Contributor

I think it would be useful to support a scenario of

someValue = dalli_client.gets('some-key')
someValue.data = 'change the value!'
dalli_client.set('some-key', someValue)

so the user doesn't even have to think about CAS.

If we return an array, the user I guess could do something like

someValue = dalli_client.gets('some-key')
someValue.first = 'change the value!'
dalli_client.set('some-key', *someValue)

but that feels more awkward. What do you think?

##
# Set the key-value pair. When 'value' is an object responding to #data
# and #cas, #set will compare-and-set the new value; this ensures safe
# updates in scenarios with multiple writers.
def set(key, value, ttl=nil, options=nil)

This comment has been minimized.

Copy link
@mperham

mperham Oct 21, 2013

Collaborator

I would rather see a new sets(key, value, cas, ttl, options) operation.

This comment has been minimized.

Copy link
@philipmw

philipmw Oct 21, 2013

Author Contributor

I was trying to avoid adding a function that doesn't reflect memcached's interface. I added 'gets' because I don't see a way to make 'get' return CAS while remaining backward-compatible, but it's possible to make 'set' support both scenarios.

Alternately, how do you feel about changing the signature of 'set' to support an optional (=0) 'cas' argument? This may break backward compatibility for the rare (I assume) users who use TTL and/or options, but it would closer reflect memcached's interface.

@philipmw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2013

@mperham, I am eager to finalize the implementation. Can you give your thoughts on my replies to your feedback?

@mperham

This comment has been minimized.

Copy link
Collaborator

commented Oct 22, 2013

I don't want to touch the existing get and set APIs. If you want to implement something, create a new API.

@philipmw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2013

Sounds good. I made the changes you suggested.

@@ -209,11 +222,20 @@ def cas(key, ttl=nil, options=nil, &block)
end
end

##
# Set the key-value pair. When 'value' is an object responding to #data
# and #cas, #set will compare-and-set the new value; this ensures safe

This comment has been minimized.

Copy link
@mperham

mperham Oct 23, 2013

Collaborator

Obsolete comment.

@mperham

This comment has been minimized.

Copy link
Collaborator

commented Oct 23, 2013

My only question now is proper naming of the methods. Are there other memcached client APIs that have get/set CAS support? If so, what do they call the operations? get_with_lock and set_with_lock seem verbose.

gets typically means return a string.

@mperham

This comment has been minimized.

Copy link
Collaborator

commented Oct 23, 2013

@philipmw Thanks for your patience with me. I see now your original PR was close to the API that spymemcached exposes. That's a big plus in my opinion so maybe we do want to expose a similar API. Specifically I'm speaking of the MemcachedClient class here: http://dustin.sallings.org/java-memcached-client/apidocs/ with its gets method and I assume the set operation knows how to unwrap a CASValue class. If you can change this to do likewise and provide specs, I'll merge.

@philipmw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2013

@mperham, I updated the pull request to be more Ruby-like. This doesn't introduce any new methods, yet remains backward compatible.

@arthurnn

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2013

I am not sure about this changes. I agree with @mperham in not touching the existent get. We should have new API for get-with-cas

@philipmw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2013

I would like CAS support for get, get_multi, and set. You propose that getcas, get_multi_cas, and set_cas is an improvement over the pull request I have now? It's fully backward-compatible, and keeping the names get and set is consistent with Memcached's own API.

@philipmw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2013

@mperham, thoughts?

@mperham

This comment has been minimized.

Copy link
Collaborator

commented Nov 2, 2013

As I said, I don't want the existing methods touched. The semantics required for CAS usage are different from normal get/set and I don't want to conflate the two into one über API.

@philipmw

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2013

@mperham, I propose two new methods: get_cas, get_multi_cas, and an update to set.
Usages:
get_cas(key) {|value, cas| ...} if a block is passed, or [value, cas] = get_cas(key) otherwise
get_multi_cas(keys) {|value, cas| ...} for each pair
set as coded in my latest pull request, where cas can be in the options hash.

Once we come to an agreement, I'll submit another pull request. Thanks!

@qianthinking

This comment has been minimized.

Copy link

commented Nov 6, 2013

Why not add cas support to 'delete' method?
Memcached support cas delete since 1.4.0 https://code.google.com/p/memcached/wiki/ReleaseNotes140

@mperham

This comment has been minimized.

Copy link
Collaborator

commented Nov 7, 2013

@philipmw That sounds like a good start.

@philipmw

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2013

@mperham, please review. @qianthinking, ok, I added CAS support for deleting.

@philipmw

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2013

@mperham, bump

@mperham

This comment has been minimized.

Copy link
Collaborator

commented Nov 16, 2013

A few concerns:

  1. All the get_multi changes. I'm not convinced we should mix get_multi and CAS at all. The resulting code feels hacky.
  2. Changing the return value for set/add/replace. It seems like the only way you should be able to get a CAS is by fetching the latest value via get_cas.
@philipmw

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2013

@mperham, thanks for the feedback.

I am motivated to expose (in a backward-compatible way) CAS where possible, the same way memcached itself exposes it in many responses.

My use of Dalli -- and I imagine anyone else's who relies on CAS -- involves storing the CAS-ID alongside every object I retrieve from memcached or set in memcached. If I need to do a safe update, I need the CAS ID associated with my set operation. Even if was efficient to do an extra round-trip to get_cas, I would not be guaranteed that the CAS-ID I retrieve is for the object I just stored. (Another writer could've set between my own set and my subsequent get_cas.)

So, I believe the only way to fulfill the spirit of CAS is to return the CAS value within the same operation.

For the same reason, it makes sense to extend CAS to get_multi. In my use of Dalli, I rely on get_multi for efficiency. Not supporting CAS there would be crippling and force users to choose between safety or speed.

@mperham

This comment has been minimized.

Copy link
Collaborator

commented Nov 18, 2013

@philipmw I understand you really want CAS. However there's been no user demand for it. You're one of maybe 2-3 people that have brought it up in the last 2 years. That's why I'm reluctant to modify heavily used existing APIs to fit the needs of very few users.

@loganb

This comment has been minimized.

Copy link

commented Nov 18, 2013

Hi Mike,

CAS is definitely an "advanced" feature. Most people make little to no attempt to keep their cache coherent, to modify objects after they have been cached, or otherwise aren't aware of the race conditions that can occur as a result, making the feature rarely used. But the folks who need it /really/ need it.

In @philipmw and my case, we fetch 100+ objects in batch, sift through them, then need to modify 1 or 2 (i.e. we're caching model-like things). CAS is the only way to safely write the modifications back, and get_multi is the only latency-safe way to fetch that many items.

I understand the concern of touching a lot of code. Can we do something to help make this patch land safely?

@mperham

This comment has been minimized.

Copy link
Collaborator

commented Nov 19, 2013

@loganb I'm fine pulling in CAS support, I just want to see changes which don't touch the existing public APIs.

@philipmw

This comment has been minimized.

Copy link
Contributor Author

commented Nov 25, 2013

@mperham, what do you think about me adding a Dalli::CasClient as the alternative to Dalli::Client? CasClient would have thorough support for CAS, including return values from add/set/replace, while not clashing with the existing API.

@mperham

This comment has been minimized.

Copy link
Collaborator

commented Nov 26, 2013

@philipmw I would prefer the API to all be in Dalli::Client. There's no reason for CAS to require a subclass, perhaps an optional require to 'activate' CAS support?

require 'dalli'
require 'dalli/cas'
Dalli::Client.new
@philipmw

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2013

@mperham, I moved all CAS functionality into its own source and test file. The only spillover is that some methods such as replace now return the new CAS value. This is an implementation detail rather than a change to the contract/interface.

@@ -163,7 +163,7 @@ def multi_response_nonblock
pos = pos + 24 + body_length

begin
values[key] = deserialize(value, flags)
values[key] = {:v => deserialize(value, flags), :cas => cas}

This comment has been minimized.

Copy link
@mperham

mperham Dec 14, 2013

Collaborator

The PR looks much better. This is my remaining issue. get_multi performance is critical and here the code is creating an additional hash for every key/value, which is immediately thrown away in get_multi. Given that 99% of people are going to be using get_multi without CAS, this is just extra overhead with no benefit for them. Is there a better way?

@philipmw

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2013

@mperham, now I use an array instead of a hash. The efficiency has just skyrocketed.

@mperham

This comment has been minimized.

Copy link
Collaborator

commented Dec 19, 2013

👍 Looks good, thanks for your patience and hard work.

mperham added a commit that referenced this pull request Dec 19, 2013

Merge pull request #398 from philipmw/get-set-with-cas
Add get-with-cas ('gets') / set-with-CAS functionality

@mperham mperham merged commit 01356cf into petergoldstein:master Dec 19, 2013

1 check failed

default The Travis CI build failed
Details
@arthurnn

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2013

❤️

@loganb

This comment has been minimized.

Copy link

commented Dec 22, 2013

Thanks for landing this Mike! Beer is on me next time you're in Seattle.

@coveralls

This comment has been minimized.

Copy link

commented Feb 16, 2015

Coverage Status

Changes Unknown when pulling 19b7728 on philipmw:get-set-with-cas into * on mperham:master*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.