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

Pass watch: kwarg to #multi calls on clients #1236

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

KJTsanaktsidis
Copy link
Contributor

Both RedisClient and RedisClient::Cluster have #multi methods that accept a watch: kwarg to watch keys as part of the transaction. See:

However, the subclasses Redis::Client and Redis::Cluster::Client in this gem(s) don't forward these keyword arguments, so you can't take advantage of them.

This PR simply delegates the watch argument.

Both RedisClient and RedisClient::Cluster have #multi methods that
accept a watch: kwarg to watch keys as part of the transaction. Expose
that argument with the wrapped subclass.
@byroot
Copy link
Collaborator

byroot commented Nov 15, 2023

Yeah, I guess it's a nice shortcut.

@byroot byroot merged commit 6729fc8 into redis:master Nov 15, 2023
16 of 17 checks passed
@KJTsanaktsidis
Copy link
Contributor Author

That's got to be the fastest merge I've ever had! thanks.

Yeah, I guess it's a nice shortcut.

It's actually a little more important than a shortcut for cluster mode - the watch/unwatch machinery in Redis::Commands::Transactions doesn't work properly with redis-cluster-client because it doesn't store transaction-related state (like what slot we have to ensure everything is in) outside of a call to #multi. So this pattern doesn't work.

$redis.watch('{hash}key1', '{hash}key2') do
    $redis.get('{hash}key1')
    # oops, don't like what I read out of key1, let's not do the transaction after all.
    $redis.unwatch
end

Using the multi(watch: ...) pattern does work though (once redis-rb/redis-cluster-client#294 gets merged).

I do want to make Redis::Cluster#watch do ... end work properly though if possible, I think that needs a little more surgery though so starting with this seemed easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants