Support watch & unwatch properly #295
Closed
Conversation
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
You might want to do things in the multi block such as read a key that
you passed to `watch:`. Currently, if you do so, you will actually read
the key _twice_. The second copy of your transaction is the one that
actually gets committed.
For example, this transaction adds _two_ to "key", not one:
```
$redis.multi(watch: ["key"]) do |m|
old_value = $redis.call_v(["GET", "key"]).to_i
m.call_v(["SET", old_value + 1])
end
```
This patch fixes the issue by batching up the transaction's calls in our
_own_ buffer, and only invoking @node.multi after the user block is
done.
This is almost certainly what you want, because otherwise the watching is totally inneffective (you might have already read a stale value from the replica that was already updated before you WATCH'd the primary)
We need this in order to get the list of keys from a WATCH command. It's probably a little overkill, but may as well implemenet the whole thing.
KJTsanaktsidis
commented
Nov 16, 2023
| # IMPORTANT: this determines the last key position INCLUSIVE of the last key - | ||
| # i.e. command[determine_last_key_position(command)] is a key. | ||
| # This is in line with what Redis returns from COMMANDS. | ||
| def determine_last_key_position(command, keys_start) # rubocop:disable Metrics/AbcSize |
There was a problem hiding this comment.
I didn't end up needing this method but it was tricky enough to write, and a natural enough analogue of determine_first_key_position, that i'm almost tempted to suggest it be committed in case it's needed in the future.
When using Redis::Cluster#watch from redis-rb, it does not actually pass the user block to RedisClient::Cluster. Instead, it actually manages the process of WATCH/UNWATCH itself, and issues call_v(['WATCH', ...]) and call_v(['UNWATCH']). To support this, we need to detect when WATCH is called and begin an "implicit transaction", within which we pin to a slot (and actually pin to a connection as well, in the case of the Pooled backend). Until UNWATCH/EXEC is issued, the connection cannot be used for access to other keys not on the transaction node. That's implemented by having the client keep a @transaction member, and set it when a WATCH command is issued.
This would be relatively simple to implement, but requires taking a dependency on concurrent-ruby, which might not be desirable.
KJTsanaktsidis
force-pushed
the
ktsanaktsidis/implicit_transactions
branch
from
6b7d487 to
1a6fcd2
Compare
supercaracal
added
bug
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is a PR which adds the concept of an "implicit transaction". The
RedisClient::Clusterobject will automatically begin a transaction if aWATCHcommand is issued, and require the user to either callUNWATCHorMULTI ... EXEC/DISCARDto close it.Between a watch and an exec/discard/unwatch, the client is locked to a particular node/connection and will reject attempts to read/write to other nodes as transaction consistency issues.
The purpose of this work is to make
RedisClient::Clusterbehave enough likeRedisClientthatwatchandmultifromRedis::Commands::Transactionsin redis-rb work with the cluster client. I outlined the current issue with that in detail in #294.With these changes in redis-cluster-client, these test-cases I wrote for redis-rb pass: redis/redis-rb@master...zendesk:redis-rb:ktsanaktsidis/cluster_txn_tests
@supercaracal , let's keep discussion of this issue in the other PR just so we don't get confused I think :)