-
Notifications
You must be signed in to change notification settings - Fork 371
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
Real Redis cluster #57
Conversation
Hi Dragan, thanks for the contribution. As far as I can see, Redis 3.0 cluster is smth different than ephemeral/sentinels in previous Redis releases. That's pretty unfortunate, as we already have the name RedisCluster in the project... https://github.com/kstyrc/embedded-redis/blob/master/src/main/java/redis/embedded/RedisCluster.java Additionally, we now need jedis jar as dependency to create a cluster and check its setup success/failure. That seems fine. Maybe I should also use jedis to check that a single node redis is up&running instead of pattern-matching std output? Would it be possible for Cluster to implement/extend Redis interface? Does it make sense? I'll comment more later regarding code review. I believe there is a couple of places to refactor/improve. |
Hi Krzysztof,
As far as I see, for now, it is absolutely different approaches which we
I agree. 3)Would it be possible for Cluster to implement/extend Redis interface? I think, yes (we can combine start and create operations or extend Cluster 4)I believe there is a couple of places to refactor/improve. Tried to do it asap, so I'm opened for discussion. Best regards,
|
2) minor bug fixing
/** | ||
* Created by dragan on 17.07.15. | ||
*/ | ||
public interface Cluster extends Redis{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should think of naming it RedisCluster. However, what could be the name for current RedisCluster with sentinels? Also, Cluster should only define create() and take start/stop/... from Redis interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but names are always problematic)...
My option is rename RedisCluster to "SentinelReplicationRedisCluster" or "RedisReplicationModel" (it should has "Replication" part as association with documentation) and "RealRedisCluster" to "RedisCluster" (as far as it is what it is)
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or just bump version of embedded-redis to 1.0
. Guys using 0.x versions will remain unaffected, and guys using new major version will have to do a little bit of work, which should be fine
Made some comments. In general, the most important points from my perspective:
As soon as we'll merge this PR. I'll make a follow-up PR with naming changes. |
|
2) push default params to builder 3) push cluster stuff to redis.embedded.cluster package 4) added cluster check to isActive()
Conflicts: src/main/java/redis/embedded/Cluster.java src/main/java/redis/embedded/cluster/RealRedisCluster.java
Redis Cluster support is based on kstyrc#57 You can still provide your own redis binary if you want to
any update on this merge and https://github.com/fmonniot/embedded-redis? |
I'm not sure if it is still valid, so I will close it. |
add support of Real Redis Cluster which is compatible with JedisCluster