Supports Multi Key commands to JedisCluster (revised of #673)

[HeartSaVioR]

(It's revised PR of #673.)

Hello.

As supporting Redis Cluster work, I implemented Cluster multi key commands based on previous interfaces.

During implementing, I found that some commands are fine with Redis, but maybe some commands are not fine with Redis Cluster.
(Actually there're no documentation from Redis describing cluster-able commands.)

So I removed some suspicious commands from interface, and introduced new interfaces for Jedis Cluster.
(Please let me know when you think something is wrong.)

Below is the commands I removed from previous interfaces.

BinaryJedisClusterCommands (copied from BinaryCommands)

List<byte[]> blpop(byte[] arg);
List<byte[]> brpop(byte[] arg);
Long move(byte[] key, int dbIndex);
MultiKeyBinaryJedisClusterCommands (copied from MultiKeyBinaryCommands)

List<byte[]> blpop(byte[]... args);
List<byte[]> brpop(byte[]... args);
Set<byte[]> keys(byte[] pattern);
String watch(byte[]... keys);
String unwatch();
byte[] randomBinaryKey();
JedisClusterCommands (copied from JedisCommands)

List blpop(String arg);
List brpop(String arg);
Long move(String key, int dbIndex);
ScanResult<Map.Entry<String, String>> hscan(final String key, int cursor);
ScanResult sscan(final String key, int cursor);
ScanResult zscan(final String key, int cursor);
MultiKeyJedisClusterCommands (copied from MultiKeyCommands)

String watch(String... keys);
String unwatch();
String randomKey();
ScanResult scan(int cursor);
ScanResult scan(final String cursor);
Set keys(String pattern);
List blpop(String... args);
List brpop(String... args);
And I introduce some utils and exception, you can find it from commit log.

Please review and comment so that this PR goes to cluster-revised.
Thanks!

[HeartSaVioR]

@vishnusaran @ReedR95 @xetorthio @marcosnils
Here's revised version of #673, so please ignore #673 and review this PR.
Thanks in advance!

[marcosnils]

@HeartSaVioR I'm reviewing this right now. You'll get my feedback soon.

[HeartSaVioR]

@marcosnils It seems that you forgot to review this. :)
We can finally merge cluster-revised branch to master when you say it's good to merge.
When can you give a feedback?

[marcosnils]

Yeah, I blocked thinking about some alternatives but I finally couldn't come up with a better solution. Between today and tomorrow I'll be done with this.

[HeartSaVioR]

@marcosnils You can share your alternative idea to realize. :)
What point do you try to improve?

Btw, we got duplicate methods from Jedis, ShardedJedis, PipelineBase, JedisCluster.
Actually it's something different (return type, getting client instance, etc.) but it will be amazing to merge into one as possible.
With Scala we can try traits, Java 1.8 we can try default method in interface.
But we're stick with Java 1.6 (though we can talk about restricting it to 1.7) so we cannot use features.

[HeartSaVioR]

@marcosnils How about merging it to 'cluster-revised' branch when we cannot find better solution right now? Do you need much time to consider better solution?
We can review 'cluster-revised' branch deeply when we're planning to release Jedis 3.0.0.

[HeartSaVioR]

Upmerged to recent cluster-revised.

[rupatel]

I would like to really thank you guys for contributing in this open source project.
"http://redis.io/topics/cluster-spec"
The above specification,in the section "Implemented subset",states that multikey operation such as "keys" are allowed as long as all keys belong to the same node.
Redis Cluster implements a concept called hash tags that can be used in order to force certain keys to be stored in the same node.
Can you please let me know when will keys command available in JedisCluster.

[HeartSaVioR]

@rupatel Hello.
"keys" command's target node is non-deterministic because there is no evidence about target (no key would be specified)
So it would be not good at Cluster mode.

In addition, AFAIK multi key operations are allowed as long as all keys belong to same "slot".

One more thing, if you're expecting Redis to process at real-time, do not use "keys".
It's for debug purposes, and we should use "scan", which is also not good at Cluster mode.

One user requests "count keys in slot" command to Redis Google Group.
(https://groups.google.com/forum/#!topic/redis-db/cin8mZuijwM)
And I added my 2 cent about "scan keys in slot".
When it will be implemented, we can provide its functionalities by Jedis Cluster.

Hope this helps.

[rupatel]

@HeartSaVioR
Hello,
Thanks for replying.
I need to store keys with say prefix Users to the same hash slot.so when I store a value against {User}1,{User}2,{User}3 ,all will be stored at the same hashslot,since redis will compute the hash for the string "User".
Now I want this functionality so that I can fetch all keys prefixing with "User" so that I can perform del or get operation on the retrieved keys.
Using Scan with Match hint will solve my issue Or may you please suggest alternative
method(In current release) to solve the above issue.

Also currently I am converting serialized java object to base64 string and then storing it to the redis server.
Is this a good practice or may be I must use separate library for SerDe or may be jedis is providing the SerDe feature.

[HeartSaVioR]

@rupatel
Scan command doesn't include slot information so Redis Cluster treats it to normal Redis command, so using it should always send "CLUSTER SLOTS" to know node's information which serves slot.

Here's some example about scan with cluster.

Hearts-MacBook-Pro:src heartsavior (unstable)$ redis-cli -p 7379
127.0.0.1:7379> set a 1
(error) MOVED 15495 127.0.0.1:7381
127.0.0.1:7379> set b 1
OK
127.0.0.1:7379> set c 1
(error) MOVED 7365 127.0.0.1:7380
127.0.0.1:7379> set d 1
(error) MOVED 11298 127.0.0.1:7381
127.0.0.1:7379> set e 1
(error) MOVED 15363 127.0.0.1:7381
127.0.0.1:7379> set f 1
OK
127.0.0.1:7379> set g 1
(error) MOVED 7233 127.0.0.1:7380
127.0.0.1:7379> set h 1
(error) MOVED 11694 127.0.0.1:7381
127.0.0.1:7379> set i 1
(error) MOVED 15759 127.0.0.1:7381
127.0.0.1:7379> set j 1
OK
Hearts-MacBook-Pro:src heartsavior (unstable)$ redis-cli -p 7380
127.0.0.1:7380> set c 1
OK
127.0.0.1:7380> set g 1
OK
Hearts-MacBook-Pro:src heartsavior (unstable)$ redis-cli -p 7381
127.0.0.1:7381> set a 1
OK
127.0.0.1:7381> set d 1
OK
127.0.0.1:7381> set e 1
OK
127.0.0.1:7381> set h 1
OK
127.0.0.1:7381> set i 1
OK
Hearts-MacBook-Pro:src heartsavior (unstable)$ redis-cli -p 7379
127.0.0.1:7379> scan 0
1) "0"
2) 1) "j"
   2) "f"
   3) "b"
127.0.0.1:7379> scan 0 match g*
1) "0"
2) (empty list or set)
127.0.0.1:7379> scan 0 match j*
1) "0"
2) 1) "j"

Redis "scan" command just shows only keys which are served from "specific" instance.

antirez will (I think he should) add scan command supporting cluster environment before releasing 3.0.0 GA.
So we don't have to make own version.

[HeartSaVioR]

@rupatel And Jedis is a blazingly "small" Redis Java client.
Jedis provides methods to communicate Redis, no more.
Spring Data Redis wraps current Redis Java client libraries (including Jedis) and support SerDe.
So you can check it out.

[rupatel]

@HeartSaVioR
Ok ,so scan command seems to returns keys from specific instance which serves the keys and not from all the instances which make up the cluster.
I need a command which can query entire key space i.e keys from all the nodes.
May you please suggest some api in JedisCluster which does this.
Or may be it is coming in next release.

[rupatel]

@HeartSaVioR
I understand that I can't query key space for every node that make up the cluater because communication between nodes is a bad idea.
But according to redis spec when I store key of the form {stuff}* redis will compute hash only for the string between braces.
So for the key it will compute hashslot for "stuff" which in turn will belong to some node in the cluster.
So the issue is how will I contact that node to fire scan command.
Does Jediscluster support hashtags.
So basically by using hash tag I need to fire scan command to that particular node so that I can retrieve keys and then by using those keys I can fire del on them.
Please Help.

[HeartSaVioR]

@rupatel
https://github.com/antirez/redis/issues/2042#issuecomment-57922528

I can only say you have to make your own, or post an issue to Redis about supporting scan inside of slot.

JedisCluster has a feature about returning connection which serves a slot based on current slot cache, but it's hidden and I cannot say exposing it will be a good idea.
We can also add a feature about updating slot cache immediately, but it could slow whole throughput at a time due to lock mechanism.
I think it's better for Redis Cluster to have scan inside of slot.

ps. Currently Redis Cluster is not for production environment. It's BETA, and not even RC yet.

[HeartSaVioR]

@rupatel I think it's not related to support multi key for now.
Could you please post an issue about it?

[rupatel]

OK,I will wait for redis to be production ready so that client don't need to implement it separately.
Sorry to pester you.

Thanks for all your efforts.
I am all agog to get my hands on such a good and useful open source project.

[HeartSaVioR]

I'm curious that we can reuse current interfaces, and throw UnsupportedOperationException if method seems not fit to Redis Cluster.
@xetorthio @marcosnils @nykolaslima
What do you think? Is it acceptable about creating new interfaces for only JedisCluster?

[nykolaslima]

I believe that it's better to have new interfaces for JedisCluster, but we can reuse some of the existing if it fits.
The reasons why I believe in that are:

1. Maybe we could have methods JedisCluster exclusive. If we share the same interface, we will have to add methods there that will only be used if it's JedisCluster. (this only applies if we could possible have JedisCluster exclusive methods)
2. Using the UnsupportedOperationException approach, the user could only have those exceptions in QA/Production. IMO, it's better to being able to verify that operation isn't supported in the compilation time, that's one of the main advantages of Java and I wouldn't like to discard it.

[HeartSaVioR]

Since we're in progress of reviewing #671, it should be included to cluster-revised branch.

[HeartSaVioR]

I'll merge this to cluster-revised branch to prepare reviewing.

[eeewhe]

@rupatel ,for example this{foo}.bar1 and another{foo}.bar2 are guaranteed to be in the same hash slot, and I want get all keys like '{foo}.xxx', but there is no jedisCluster function like 'keys', so you have idea or some jedis solutions now ? Because of your Q&A above, could you give me some suggestions?

[rupatel]

Hey, We have to wait until they implement and place this feature in new release. Other option is to implement feature, I tried by using one of the api they had exposed to fetch the slot in which the current pattern is stored, but it is not a good design to iterate  all nodes in  cluster and fetch the keys since we need to handle failover and other cases.

So I have not yet cracked the problem, if you find any solution please let me know.

From:"Guocai He" notifications@github.com
Date:Wed, Aug 12, 2015 at 10:56 AM
Subject:Re: [jedis] Supports Multi Key commands to JedisCluster (revised of #673) (#687)

@rupatel ,for example this{foo}.bar1 and another{foo}.bar2 are guaranteed to be in the same hash slot, and I want get all keys like '{foo}.xxx', but there is no jedisCluster function like 'keys', so you have idea or some jedis solutions now ? Because of your Q&A above, could you give me some suggestion?

—
Reply to this email directly or view it on GitHub.