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

Remove ShardedJedis in order to focus core functionality (3.x) #875

Closed

Conversation

HeartSaVioR
Copy link
Contributor

@HeartSaVioR HeartSaVioR commented Feb 3, 2015

https://groups.google.com/d/msg/jedis_redis/7ZJPFp_qTY0/mIZ9qZ8ZgdkJ

We're decided to stop supporting ShardedJedis starting from 3.x. (with leaving it to 2.x)

I removed ShardedJedis with relevant codes and try to clean codes to make Jedis clearer.

At first, many classes are related to ShardedJedis including utilities, so we can get rid of it.
And we can simplify Pipeline and relevant interfaces because there's no need to split operations which one is single key and other one is multi keys.

In addition, I've modified some interfaces name cause to have consistent naming convention.
#630 also removes ShardedJedis, but it removes only ShardedJedis and relevant codes, not modifying interfaces, and Pipeline.


This change is Reviewable

@HeartSaVioR
Copy link
Contributor Author

@xetorthio @marcosnils @nykolaslima
Please review with comparing to #630, and comment. Thanks!

@HeartSaVioR HeartSaVioR added this to the 3.0.0 milestone Feb 3, 2015
@nykolaslima
Copy link
Contributor

@HeartSaVioR I don't know too much about Pipeline part, but the PR looks good to me.

I liked the names' refactoring. But I think that after merging it, we can begin to refactor the packages. We have a single jedis package and many classes inside it. We can split it in many domain specific packages.

Great job removing all this 👍

@HeartSaVioR
Copy link
Contributor Author

Regarding Pipeline, if we remove ShardedJedis, we are no longer need to separate single key and multi key operations by Pipeline/Transaction.
We still need to separate single key and multi key operations by normal situation because of JedisCluster.
Some multi-key operations seems not fit to JedisCluster, so we can't use same interfaces, which we need to discuss how to solve.

* Remove ShardedJedis and relevant classes, methods
** also remove unit tests / benchmark tests
* Rearrange Pipeline to reflect removing ShardedJedis …
** We don't need to separate single key and multi keys operation
*** cause JedisCluster can't use Pipeline by nature
** Rename interfaces to respect naming convention
** and so on...
@HeartSaVioR
Copy link
Contributor Author

@marcosnils @sazzad16
I still think removing ShardedJedis brings benefits on codebase (as you can see the count of deleted lines from the diff). Redis introduces Redis Cluster from 3.0 and Redis even reached 4.0. Let's recommend Redis Cluster and just remove ShardedJedis.

Copy link
Collaborator

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

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

@HeartSaVioR Whether to keep ShardedJedis or not should somehow depend on users. There is hardly any way to know that, at least for me.

IMHO, deprecation with proper documentation would be a safer option. And if we really want to remove ShardedJedis, we may have to think about the alternatives for the users.

@HeartSaVioR
Copy link
Contributor Author

@sazzad16
Yeah I just forgot #629. Please follow up the discussion on #629.

Reminding the discussion, there may be a room for ShardedJedis which Redis Cluster doesn't cover. (Don't know if it is still valid.) But as @antirez stated at later comments, if the Jedis codebase is being complicated because of ShardedJedis, it can be splitted out of. It can be a new project, or we could have multi-module projects and let ShardedJedis out of core. For now ShardedJedis is integrated into Jedis class (see constructor) and coupled with interfaces, which is the reason of being able to remove huge codes only for removing ShardedJedis.

If we could provide consistent-hashing with ShardedJedis, I would have no objection to its worth. As @antirez commented, actually, ShardedJedis may be more complicated to have unique worth against Redis Cluster. I'm not sure any of us volunteers, so that's another reason to move out of or even remove.

@gkorland gkorland modified the milestones: 3.0.0, 3.1.0 Dec 6, 2018
@sazzad16 sazzad16 modified the milestones: 3.1.0, 4.0.0 Dec 6, 2018
@sazzad16 sazzad16 removed this from the 4.0.0 milestone Feb 6, 2021
@sazzad16
Copy link
Collaborator

sazzad16 commented Dec 8, 2021

Resolved by #2693

@sazzad16 sazzad16 closed this Dec 8, 2021
@sazzad16 sazzad16 added this to the 4.0.0 milestone Dec 8, 2021
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.

4 participants