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 cluster scan implementation #1049

Merged
merged 1 commit into from
Nov 19, 2021

Conversation

estepnv
Copy link
Contributor

@estepnv estepnv commented Nov 6, 2021

Fixes #1045

@estepnv
Copy link
Contributor Author

estepnv commented Nov 6, 2021

please suggest what additional tests should I add

@estepnv estepnv changed the title Fixes Fixes redis/redis-rb#1045 Add cluster scan implementation Nov 6, 2021
lib/redis/cluster.rb Outdated Show resolved Hide resolved
@supercaracal
Copy link
Contributor

redis/redis#1962 (comment)

Redis server maintainer said:

I don't think that --scan and --cluster modes play nicely together.

I don't know that how much the feature is needed.
But current redis gem's behavior is a bug indeed. Randomly assign is nonsense.

https://github.com/RedisLabs/redis-cluster-proxy#commands-that-act-differently-from-standard-redis-commands-or-that-have-special-behavior

SCAN: performs the scan on all the master nodes of the cluster.
The cursor contained in the reply will have a special four-digits suffix indicating the index of the node that has to be scanned.
Note: sometimes the cursor could be something like "00001", so you mustn't convert it to an integer when your client has to use it to perform the next scan.

The above proxy feature supports SCAN command by similar interface.
But it seems that the implementation is more simply.

https://github.com/RedisLabs/redis-cluster-proxy/blob/ac83840d1108901a475e24232b7a0986e97e65dd/src/proxy.c#L812-L819

Scan all master nodes in the cluster.
Node order is alphabeticallty and it's taken from clusterGetMasterNames.
The index of the node is taken from the cursor last 4 digits.
If the index is beyond the list of all the master the choosen node will be the last one.
If the cursor is '0', the choosen node will be the first one.
This special cursor format will be handled by handleScanReply, that will append the index of the node to scan to the cursor replied from the cluster.
The real cursor sent to the cluster won't actually contain the index suffix.

https://redis.io/commands/scan
Also, It seems that redis server has just a state as a cursor.

Is it better if we implement it by stateless logic?

@supercaracal
Copy link
Contributor

I am just a contributor. It is eventually needed to review by maintainer.

@supercaracal
Copy link
Contributor

supercaracal commented Nov 13, 2021

approach 1

  • Client raises an error when calling public #scan method in cluster mode.
  • Only public #scan_each method is legitimate when using cluster mode.
  • It needs conditional branching in public #scan_each method.
  • Client internally iterates with double loop by nodes and cursors.
  • It might be a breaking changes even if the purpose is a bug fixing.

approach 2

  • Client knows ordered nodes.
    • ex) 127.0.0.1:7000, 127.0.0.1:7001, ...
  • At first, we can call public #scan method with 0.
  • Client returns next cursor and scanned keys. The cursor includes current node index.
    • such that {cursor}-{node index}
    • ex) 17-0, 0-1, 42-1, 0-2, ...
  • We can iterate every nodes using only cursor value.
    • Client returns next node cursor when current node iteration is finished.
    • Client returns 0 when all iterations are finished.

I'd say that the approach 2 may be ideal.

lib/redis/cluster.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@supercaracal supercaracal left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

@byroot byroot left a comment

Choose a reason for hiding this comment

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

Please squash your commits together and I'll merge.

lib/redis/cluster.rb Outdated Show resolved Hide resolved
lib/redis/cluster.rb Outdated Show resolved Hide resolved
lib/redis/cluster.rb Outdated Show resolved Hide resolved
Fix rubocop offences

Implement stateless cluster scan

Isolate clients access in the node computed property
@@ -137,6 +137,7 @@ def send_command(command, &block)
when 'wait' then @node.call_master(command, &block).reduce(:+)
when 'keys' then @node.call_slave(command, &block).flatten.sort
when 'dbsize' then @node.call_slave(command, &block).reduce(:+)
when 'scan' then _scan(command, &block)
Copy link
Contributor Author

@estepnv estepnv Nov 18, 2021

Choose a reason for hiding this comment

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

@byroot what about zscan/hscan/sscan, will it work? maybe I should add more tests? I'm not very familiar with internals.

Copy link
Collaborator

Choose a reason for hiding this comment

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

More tests wouldn't hurt.

@byroot byroot merged commit 543beb2 into redis:master Nov 19, 2021
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.

Cluster mode #scan is not working properly
3 participants