Jedis Cluster functionalities improved (cluster-revised branch)

[HeartSaVioR]

Since there're some PRs on JedisCluster and it will be merged on 3.0.0, we can incubate cluster-revised branch for improving JedisCluster.

Currently these PRs are merged to cluster-revised branch (updated continuously)

• Remove BasicCommand implementation from JedisCluster #617
  • Remove BasicCommand implementation from JedisCluster
• BinaryJedisCommands, ScriptingCommands, BinaryScriptingCommands #618
  • BinaryJedisCommands, ScriptingCommands, BinaryScriptingCommands

[HeartSaVioR]

So many random tests failure occurred... We should have an issue tracing these and try to fix / discard tests.

[HeartSaVioR]

@marcosnils
I've merged multi key ops PR into this PR, and upmerged.
We still don't have tests, and I wonder we can test commands for normal, pipeline, transaction, cluster, etc without much duplicated code.

[HeartSaVioR]

Please note that I spent another over 1 hours to upmerge #687 and this.
I REALLY don't want to try it again.

[HeartSaVioR]

@christophstrobl
I introduced new interfaces for BinaryJedisCluster and JedisCluster.
BinaryJedisCluster and JedisCluster don't have move() now. :)

[christophstrobl]

thanks @HeartSaVioR!

[marcosnils]

@HeartSaVioR exceptional work, I don't have preliminary observations. This PR really takes us really close to have a 3.0 version!.

[HeartSaVioR]

@marcosnils Thanks for reviewing huge PR! Great work!

[benkris1]

@marcosnils @HeartSaVioR @christophstrobl you guys do a great job. great programer! Thank all of you!

[bpoweski]

Is the intent to remove any common protocol between Jedis/JedisCluster for the 3.0 version?

[marcosnils]

@bpoweski what do you mean exactly by "removing common protocols"?

[bpoweski]

Apologies, I meant interfaces!

In the 2.7.3 branch JedisCluster and Jedis have the following signatures.

public class JedisCluster implements JedisCommands, BasicCommands, Closeable 

public class Jedis extends BinaryJedis implements JedisCommands, MultiKeyCommands,
    AdvancedJedisCommands, ScriptingCommands, BasicCommands, ClusterCommands, SentinelCommands 

In master they now have:

public class JedisCluster extends BinaryJedisCluster implements JedisClusterCommands,
        MultiKeyJedisClusterCommands, JedisClusterScriptingCommands

public class Jedis extends BinaryJedis implements JedisCommands, MultiKeyCommands,
        AdvancedJedisCommands, ScriptingCommands, BasicCommands, ClusterCommands, SentinelCommands

While there are various caveats for using Redis cluster, the underlying API is mostly the same. Having a common interface between the two clients is useful. For example, we run our application using both a Redis Cluster and a non clustered Redis configuration. We currently use JedisCommands to abstract most of the differences between the two connections.

[HeartSaVioR]

@bpoweski
There was some comments about removing caveats (some commands are not compatible with Redis Cluster) at compile time, and I think it is still important.
I also think having same interface is awesome for abstraction, but filling method body to UnsupportedOperationException is also bad kind of implementation.

tl;dr. There's pros and cons about each approach, and I just pick first one. It could be changed anytime when there's elegant way to resolve.

Also please note that Jedis 3.0 should be breaking changes, so we're free to REWRITE whole codes though I think it's hard to be occured for now.

[bpoweski]

Have you considered rolling up shared signatures into a common interface and then using extends. For example, changing from:

public interface JedisCommands {
  String set(String key, String value);
  ..
}

public interface JedisClusterCommands {
  String set(String key, String value);
  ..
}

to:

public interface SingleKeyCommands {
  String set(String key, String value);
}

public interface JedisCommands extends SingleKeyCommands {
  ..
}

public interface JedisClusterCommands extends SingleKeyCommands, OtherSpecificNonClusterAllowedCommands {
  ..
}

An added benefit is that it would facilitate decomposing the larger JedisCommands and JedisClusterCommands interfaces into ones grouped by function. I suspect you'd reduce the line count by doing so given there is a so much overlap.

[HeartSaVioR]

@bpoweski Maintaining OtherSpecificNonClusterAllowedCommands should be annoying.
I'm thinking another approach, breaks interface into data type, though it is just an idea for now.

[HeartSaVioR]

@bpoweski Forgot to say "Thanks for giving your opinion!" :)
Please feel free to tell us your idea when there's better approaches.

[bpoweski]

@HeartSaVioR, thank you for your work on this project.

[lcy362]

when will 3.0.0 release?