finagle-redis: Redis cluster management #645
Conversation
Problem Unit tests and integration tests are mixed in the finagle-redis project under the test path. In addition, the SBT configuration is only running unit tests. This solves issue twitter#360. Solution Added configuration for IntegrationTest in the finagle-redis project and moved integration tests to the correct path. Redis process shutdown always includes all processes. Result Running 'test' only executes unit tests. Running 'it:test' executes the integration tests.
Problem Unit tests and integration tests are mixed in the finagle-redis project under the test path. In addition, the SBT configuration is only running unit tests. This solves issue twitter#360. Solution Added configuration for IntegrationTest in the finagle-redis project and moved integration tests to the correct path. Redis process shutdown always includes all processes. Result Running 'test' only executes unit tests. Running 'it:test' executes the integration tests.
- New commands: MIGRATE, SETSLOT, GETKEYSINSLOT
|
Note that this PR includes #642 |
Codecov Report
@@ Coverage Diff @@
## develop #645 +/- ##
===========================================
- Coverage 67.46% 67.18% -0.28%
===========================================
Files 644 647 +3
Lines 21843 21937 +94
Branches 1689 1672 -17
===========================================
+ Hits 14736 14738 +2
- Misses 7107 7199 +92
Continue to review full report at Codecov.
|
mkhq
changed the title
…q/finagle into redis-enable-integration-tests-final
|
Looks good to me, but this is honestly the first time I've looked at redis technology so I'm far from an expert. If we can get another set of eyes or two, I think it's ready to go. Ping @vkostyukov, who I believe knows more than I do about this protocol implementation. |
| } | ||
|
|
||
| // jump past the '\r\n' | ||
| reader.skip(2) |
There was a problem hiding this comment.
How do we know \n follows \r? I don't see a check for that.
There was a problem hiding this comment.
Its an assumption from the protocol: https://redis.io/topics/protocol#bulk-string-reply
|
@vkostyukov Thanks for the review! I'll fix the visibility of ClusterClient. In the meantime I've been working on another PR for transparently handling cluster slot redirect and management. Will try to submit soonish. |
|
@mkhq, perhaps you're already going to do this, but it would be helpful if you could rebase+squash the PR when you push the commit for visibility. (It makes it easier to merge internally, but its not a show stopper) |
|
|
|
@bryce-anderson Hey, I already merged in develop in the branch so rebase+squash would be a bit of a mess. Should I create a new branch/PR? |
|
Ping @mosesn |
|
@mkhq yeah, a new branch/PR would be great, if you can. Sorry for the delay! |
|
@mkhq, I'm really sorry I didn't see your previous ping: a lot of things got lost in my inbox during that time period. |
|
@bryce-anderson no worries :) I didnt have time to fix it until now |
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.
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.