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

finagle-redis: Redis cluster management #668

Closed
wants to merge 1 commit into from

Conversation

mkhq
Copy link
Contributor

@mkhq mkhq commented Jan 12, 2018

Problem

The current finagle-redis client does not have support for the CLUSTER commands introduced in Redis 3.0.0.

Solution

This adds support for a number of new cluster commands that are necessary to setup and manage a redis cluster.

  • CLUSTER ADDSLOT, assigns a storage slot to a server
  • CLUSTER SETSLOT, updates the state of a slot
  • CLUSTER SLOTS, returns a list of slots known to the server
  • CLUSTER GETKEYSINSLOT, returns keys stored in a slot
  • CLUSTER REPLICATE, changes a node to become a replica of a server
  • CLUSTER MEET, introduces cluster nodes to each other
  • CLUSTER NODES, returns the list of cluster nodes known to the server
  • CLUSTER INFO, returns the cluster specific info
  • MIGRATE, used to transfer keys to another node

Result

Using the introduced commands it is possible to setup a redis cluster for testing and live re-sharding. This is the first step in implementing the redis cluster client protocol. The next step is to handle operation redirects, i.e. when the server returns ASK or MOVED.

Problem

The current finagle-redis client does not have support for the CLUSTER commands introduced in Redis 3.0.0.

Solution

This adds support for a number of new cluster commands that are necessary to setup and manage a redis cluster.

- CLUSTER ADDSLOT, assigns a storage slot to a server
- CLUSTER SETSLOT, updates the state of a slot
- CLUSTER SLOTS, returns a list of slots known to the server
- CLUSTER GETKEYSINSLOT, returns keys stored in a slot
- CLUSTER REPLICATE, changes a node to become a replica of a server
- CLUSTER MEET, introduces cluster nodes to each other
- CLUSTER NODES, returns the list of cluster nodes known to the server
- CLUSTER INFO, returns the cluster specific info
- MIGRATE, used to transfer keys to another node

Result

Using the introduced commands it is possible to setup a redis cluster for testing and live re-sharding. This is the first step in implementing the redis cluster client protocol. The next step is to handle operation redirects, i.e. when the server returns ASK or MOVED.
@mkhq
Copy link
Contributor Author

mkhq commented Jan 12, 2018

@mosesn See #645 for previous discussion.

@codecov-io
Copy link

codecov-io commented Jan 12, 2018

Codecov Report

Merging #668 into develop will decrease coverage by 0.14%.
The diff coverage is 6.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #668      +/-   ##
===========================================
- Coverage    71.73%   71.58%   -0.15%     
===========================================
  Files          723      726       +3     
  Lines        23016    23110      +94     
  Branches      1812     1847      +35     
===========================================
+ Hits         16511    16544      +33     
- Misses        6505     6566      +61
Impacted Files Coverage Δ
...tter/finagle/redis/protocol/commands/Cluster.scala 0% <0%> (ø)
.../scala/com/twitter/finagle/redis/KeyCommands.scala 0% <0%> (ø) ⬆️
...la/com/twitter/finagle/redis/ClusterCommands.scala 0% <0%> (ø)
...la/com/twitter/finagle/redis/util/TestServer.scala 0% <0%> (ø) ⬆️
...cala/com/twitter/finagle/redis/ClusterClient.scala 0% <0%> (ø)
...a/com/twitter/finagle/redis/protocol/Command.scala 100% <100%> (ø) ⬆️
...twitter/finagle/redis/protocol/commands/Keys.scala 100% <100%> (ø) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a205bd2...f1eed12. Read the comment docs.

@ryanoneill
Copy link
Contributor

Hi @mkhq, thanks for the pull request. I'll get someone who knows finagle-redis better than me to take a look soon.

@ryanoneill
Copy link
Contributor

Sorry, my mistake. I now see the context from the other issue / pull request. I'm going to put this up for review internally and run against our existing internal code. Thanks.

@mkhq
Copy link
Contributor Author

mkhq commented Jan 22, 2018

@ryanoneill thanks, sounds good 👍 Just a note on running the integration tests, the cluster protocol was introduced in Redis 3.2. I've tested it against Redis 4.x (and 3.x) and it worked fine.

@oscar-stripe
Copy link

this would be useful to me.

@ryanoneill any update on when Twitter can review?

@kevinoliver
Copy link
Contributor

i think @ryanoneill got busy on some other work. we'll find someone else to take a look. thanks for your patience.

@kevinoliver
Copy link
Contributor

i think @ryanoneill got busy on some other work. we'll find someone else to take a look. thanks for your patience.

it me.

working on getting this pulled in, apologies for the delay and thanks for your patience.

@kevinoliver
Copy link
Contributor

This got merged in 86b151b

Thanks again @mkhq.

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

Successfully merging this pull request may close these issues.

None yet

5 participants