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

RedisCluster -> Mutable/Immutable #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jhiggins-bam
Copy link

Hello!

This change addresses such issues as etaty#177 .

Essentially it is for supporting Redis clusters (in "cluster mode").

The previous versions of rediscala support cluster mode, but the RedisCluster class only accepts an immutable list of servers, and as such the list of nodes cannot change, however this does not allow the user to utilize Redis cluster features such as scaling out shards, adding or removing replication nodes, etc.

This changes RedisCluster to an abstract class. From there, I added ImmutableRedisCluster which is an implementation of RedisCluster that maintains the same functionality as in previous versions, and MutableRedisCluster, which is an abstract class that will refresh the list of nodes every n seconds, where the method of refreshing the list of nodes depends on the implementation.

I have also added an implementation of MutableRedisCluster, ClusterCommandRedisCluster, which uses the CLUSTER NODES command to refresh the list of nodes. This class accepts a configUrl as input which is based on the AWS Elasticache flavor of Redis clusters, but more generically the configUrl is a CNAME record from which the initial nodes can be discovered.

This is a breaking change! Suggestions on how to make this a non-breaking change are welcomed (other suggestions are welcomed, too).

@Ma27
Copy link
Owner

Ma27 commented Sep 25, 2018

Hi!
First of all, thanks a lot for your effort!
Unfortunately I'll be a bit busy the next days, so please be a bit patient with my review :)

/cc @kardapoltsev @herzrasen this might be interesing for you as well

redisServerConnections.keys.toSeq
}

def addServer(server: RedisServer) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Procedure syntax is deprecated


protected val timeoutMillis: Option[Long] = timeout.map(_.toMillis)

if(timeout.isDefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like foreach

@@ -1 +1 @@
version := "1.8.4"
version := "2.0.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it shouldn't be a part of PR

@Ma27
Copy link
Owner

Ma27 commented Jan 7, 2019

first of all sorry, due to several time limitations I neglected this repository far too much. I'm not sure when exactly, but I hope that I can find some time to do a review here at one of the next weekends.

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.

3 participants