Add a flag to returns the CAS calue from ADD/SET/REPLACE/CAS and multi-set operations#261
Open
alexmv wants to merge 3 commits intojaysonsantos:masterfrom
Open
Add a flag to returns the CAS calue from ADD/SET/REPLACE/CAS and multi-set operations#261alexmv wants to merge 3 commits intojaysonsantos:masterfrom
alexmv wants to merge 3 commits intojaysonsantos:masterfrom
Conversation
ReplicatingClient's CAS-touching methods have always had a silent correctness problem when used against more than one replica: each server maintains its own CAS counter, so a CAS value cannot match on more than one replica. any(returns) then reports success as long as one server accepted the write, but the other replicas silently rejected it, leaving them divergent. The get-side methods do not warn about this -- `gets()`, `get(get_cas=True)`, and `get_multi(get_cas=True)` return a CAS from whichever replica happened to respond first, even though that value cannot be safely passed to cas() on a multi-replica client. Add warnings on the class docstring and on each affected method, so callers have some hope of noticing the hazard. For backwards compatibility, the behavior itself is left unchanged.
add(), set(), replace(), and cas() all produce an item with a new CAS value on success, and the memcached binary protocol already returns it in the response header -- the client was simply discarding it. Callers who want to chain a CAS-guarded update after a write had to follow up with a separate gets() round-trip, which is both slower and racy (another writer could slip in between):. Add an optional `get_cas=False` kwarg matching the existing convention on get()/get_multi(). When True, these methods now return a tuple of `(success, cas)` instead of a plain bool; `cas` is the new CAS on success, or None on failure. For ReplicatingClient, the returned CAS comes from the first replica that reported success, matching how get/gets already handle replicas. CAS is inherently per-server, so only that server's CAS is meaningful for subsequent CAS operations against the same server.
set_multi's current return shape is a list of failed keys, which can't
carry a per-key CAS value. It also uses the quiet setq/addq opcodes,
which intentionally suppress successful responses -- so even if the
shape allowed it, the wire protocol wouldn't return a CAS per key.
Add a separate `set_multi_cas` method that uses the non-quiet set/add
opcodes (one response per key) and returns `{str_key: int | None}` for
every input key -- int on success, None on failure. The existing
`{(key, cas): value}` input syntax from set_multi is preserved; the
result dict is keyed by the string key regardless of which form was
passed.
For ReplicatingClient, the returned CAS per key is the first non-None
CAS from any replica, matching the single-key helpers.
Contributor
Author
|
Pushed #262 to fix CI. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The binary protocol returns the CAS value from these operations already -- but it's currently being silently discarded. Being able to get the value back without an extra GETS reduces latency by dropping a round-trip, and also closes a class of race condition.
I've one this in a way which is fully backwards-compatible, since the API for this module is quite stable. The first commit in the series calls out that
ReplicatingClient.cas()is pretty much guaranteed to cause drift in the values stored in the "replicas," so is almost certainly never what you want. For backwards-compatibility, I left this as a documentation-only change -- but it would be safer to make it raiseNotImplementedError. Let me know what you'd prefer.