Skip to content

Conversation

@roirex
Copy link

@roirex roirex commented Jan 5, 2015

  • added support for scan commands (incl. ScanParams) in JedisCluster.java by sending scan command to Master nodes (only) and merge answers using multiple threads.
  • fixed bug/type for hscan, sscan and zscan commands in JedisCluster.java
  • added support for ScanParams for hscan, sscan and zscan in JedisCluster.java and ShardedJedis.java
  • added/fixed test case for new and changed hscan, sscan, zscan commands and for added scan command
  • added missing test case for blpop and brpop

@HeartSaVioR
Copy link
Contributor

@roirex At first, thanks for your effort!

Btw, any cluster enhancements are now in branch, 'cluster-revised'.
PR is here, #671, and we're in progress of reviewing.

In that PR, we removed implementation of BasicCommands from JedisCluster, so some of your enhancements are no longer valid in current.

And empty ip in "cluster nodes" never means localhost.
Please refer https://github.com/antirez/redis/issues/1848, and #699, and HeartSaVioR@0f472c9

So finally, your changeset has some invalid implementations, so I'll close this PR.
Please refer 'cluster-revised' branch and apply your effort. Thanks!

@HeartSaVioR HeartSaVioR closed this Jan 5, 2015
@roirex
Copy link
Author

roirex commented Jan 7, 2015

Thanks for your feedback. I'll redo my effort in cluster-revised.

However I don't follow you when you say "we removed implementation of BasicCommands from JedisCluster, so some of your enhancements are no longer valid in current."
Which ones? If I look in BasicCommands I don't see any of the methods I have changed brpop, blpop, zscan, hscan, sscan and scan.

@HeartSaVioR
Copy link
Contributor

@roirex Oh, sorry. scan is in MultiKeyCommands.
Btw, in #687 I removed non-target multi-keys (actually no keys) commands, so my changeset and your PR will be conflicted.

We're discussing about handling non-target commands, and I think we should remove these.
Please refer #764

@roirex roirex deleted the cluster_enhancements branch January 8, 2015 08:49
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.

2 participants