Skip to content

Fix the watch command bugs for the cluster client #1255

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 15, 2024
Merged

Fix the watch command bugs for the cluster client #1255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cluster/lib/redis/cluster.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ def cluster(subcommand, *args)
send_command([:cluster, subcommand] + args, &block)
end

def watch(*keys, &block)
synchronize { |c| c.call_v([:watch] + keys, &block) }
end

private

def initialize_client(options)
Expand Down
2 changes: 1 addition & 1 deletion cluster/redis-clustering.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,5 +47,5 @@ Gem::Specification.new do |s|
s.required_ruby_version = '>= 2.7.0'

s.add_runtime_dependency('redis', s.version)
s.add_runtime_dependency('redis-cluster-client', '>= 0.7.0')
s.add_runtime_dependency('redis-cluster-client', '>= 0.7.11')
end
21 changes: 21 additions & 0 deletions cluster/test/client_transactions_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,25 @@ def test_cluster_client_does_not_support_transaction_by_multiple_keys
assert_nil(redis.get("key#{i}"))
end
end

def test_cluster_client_does_support_transaction_with_optimistic_locking
redis.mset('{key}1', '1', '{key}2', '2')

another = Fiber.new do
cli = build_another_client
cli.mset('{key}1', '3', '{key}2', '4')
cli.close
Fiber.yield
end

redis.watch('{key}1', '{key}2') do |tx|
Copy link

@KJTsanaktsidis KJTsanaktsidis Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the right interface that redis-cluster-client/redis-clustering should be exposing.

The normal redis-rb client interface looks like this:

redis.watch('{key}1', '{key}2') do
    redis.get('{key}1')
    redis.get('{key}2')
    # Make some decision about what to do based on the value of these keys
   redis.multi do |tx|
       tx.set('{key}1', 'new_v1')
       tx.set('{key}2', 'new_v2')
    end
end

The differences are...

  • watch does not actually yield anything (EDIT: it yields self but there are plenty of documented examples where the yielded value is ignored)
  • A transaction is not actually started unless multi is called

Would you like me to put up a different pair of PR's (one here, one in redis-cluster-client) to try and show how I think we could implement it in a compatible way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my implementation, the block is finally passed to the redis-cluster-client gem. I think the redis-cluster-client gem should not be complex in excess due to the redis gem. I think the transaction feature is quite different between standalone and cluster. But of course, I think the other approaches are welcome.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I can put most of the "hacking"/complexity in redis-clustering, but keep an interface in redis-cluster-client that actually makes sense in cluster mode.

Basically, redis-clustering is the "adapter" that takes the redis-cluster-client interface and makes it fit the redis-rb API pattern. That seems to make the most sense to me.

Let me try and get a couple of PR's up today :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@supercaracal what do you think about this approach? #1256 and redis-rb/redis-cluster-client#339

Basically, exposing the OptimisticLocking object from a dedicated #watch API on the redis-cluster-client side and then redis-rb makes that state implicit through the @active_watcher ivar.

another.resume
v1 = redis.get('{key}1')
v2 = redis.get('{key}2')
tx.call('SET', '{key}1', v2)
tx.call('SET', '{key}2', v1)
end

assert_equal %w[3 4], redis.mget('{key}1', '{key}2')
end
end
25 changes: 22 additions & 3 deletions cluster/test/commands_on_transactions_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,29 @@ def test_unwatch
end

def test_watch
assert_raises(Redis::CommandError, "CROSSSLOT Keys in request don't hash to the same slot") do
redis.watch('key1', 'key2')
assert_raises(Redis::Cluster::TransactionConsistencyError) do
redis.watch('{key}1', '{key}2')
end

assert_equal 'OK', redis.watch('{key}1', '{key}2')
assert_raises(Redis::Cluster::TransactionConsistencyError) do
redis.watch('key1', 'key2') do |tx|
tx.call('SET', 'key1', '1')
tx.call('SET', 'key2', '2')
end
end

assert_raises(Redis::Cluster::TransactionConsistencyError) do
redis.watch('{hey}1', '{hey}2') do |tx|
tx.call('SET', '{key}1', '1')
tx.call('SET', '{key}2', '2')
end
end

redis.watch('{key}1', '{key}2') do |tx|
tx.call('SET', '{key}1', '1')
tx.call('SET', '{key}2', '2')
end

assert_equal %w[1 2], redis.mget('{key}1', '{key}2')
end
end
2 changes: 1 addition & 1 deletion test/helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ def version
def with_acl
admin = _new_client
admin.acl('SETUSER', 'johndoe', 'on',
'+ping', '+select', '+command', '+cluster|slots', '+cluster|nodes',
'+ping', '+select', '+command', '+cluster|slots', '+cluster|nodes', '+readonly',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This permission may be needed. I don't know why it becomes needed since when. Some cases became failure in the ACL test.

'>mysecret')
yield('johndoe', 'mysecret')
ensure
Expand Down