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

Deprecating Sharded Jedis (for 2.5.0) #629

Closed
wants to merge 3 commits into from

Conversation

HeartSaVioR
Copy link
Contributor

It's for completely removing ShardedJedis at 3.0.0.

Discussion for deprecating starts at https://groups.google.com/d/msg/jedis_redis/avphfQld81Y/X_uouHp_lCIJ.
Although it couldn't get many "agreement"s, but no "disagreement" received.
So we will start removing ShardedJedis.

@xetorthio @marcosnils Please review and comment! Thanks!

@HeartSaVioR HeartSaVioR changed the title Deprecating Sharded Jedis Deprecating Sharded Jedis (for 2.5.0) Apr 29, 2014
@HeartSaVioR HeartSaVioR mentioned this pull request Apr 29, 2014
@marcosnils
Copy link
Contributor

@HeartSaVioR @xetorthio I think it'd be nice to add a @Deprecated comment before merging this PR. It's quite shocking to see that all sharded methods will be removed upon updating a library without seeing a comment in the code.

What about adding a short description of why we're deprecating and a link to the google groups thread in the ShardedJedis class at least?

@HeartSaVioR
Copy link
Contributor Author

@marcosnils Yes, you're right.
Actually many users seem to use ShardedJedis, so it's very big impact on users.
I'll add description with reason.
Thanks for finding a missed spot! :)

@xetorthio
Copy link
Contributor

I wonder why we decided to remove it. Following semver we should only
introduce breaking changes on a major release, and we didn't have one in a
long long time :)
So yes. Lets only add @deprecated and maybe schedule an issue to remember
to remove it in jedis 3.0

On Sun, May 25, 2014 at 7:02 PM, Jungtaek Lim notifications@git.luolix.topwrote:

@marcosnils https://github.com/marcosnils Yes, you're right.
Actually many users seem to use ShardedJedis, so it's very big impact on
users.
I'll add description with reason.
Thanks for finding a missed spot! :)


Reply to this email directly or view it on GitHubhttps://github.com//pull/629#issuecomment-44148138
.

@HeartSaVioR
Copy link
Contributor Author

@xetorthio @marcosnils Didn't we decided to remove at 3.0.0?
If we have not fully decided, then I'll modify comment to just mention about deprecating, not removing.
Please give me a feedback. Thanks.

@marcosnils
Copy link
Contributor

@xetorthio. My understanding and @HeartSaVioR's was that we wanted to
remove sharded redis for 3.0 as JedisCluster accomplishes the same
functionality.

That's why we are deprecating it for 2.5 so we can remove it completely in
3.0

sent from mobile
El may 25, 2014 11:44 p.m., "Jungtaek Lim" notifications@github.com
escribió:

@xetorthio https://github.com/xetorthio @marcosnilshttps://github.com/marcosnilsDon't we decided to remove at 3.0.0?
If we have not fully decided, then I'll modify comment to just mention
about deprecating, not removing.
Please give me a feedback. Thanks.


Reply to this email directly or view it on GitHubhttps://github.com//pull/629#issuecomment-44153460
.

@HeartSaVioR
Copy link
Contributor Author

@marcosnils So do I.
@xetorthio
I think if we're not going to remove ShardedJedis at 3.0.0, deprecating ShardedJedis is too early.
When we mark ShardedJedis to "deprecated", users would not using ShardedJedis as time goes by.
(It would be nearly same to removing, as our intention to deprecating.)
And so do we, then ShardedJedis would be omitted for later modification, and eventually when we decided to cancel deprecated, problem will arise.

Actually we can implement Consistent Hashing and let ShardedJedis give a new life.
Please see #645.
As you know, RedisCluster cannot replace Consistent Hashing, so it would be unique feature.

@HeartSaVioR
Copy link
Contributor Author

@xetorthio @marcosnils
Actually, and ideally it lacks more opinions on this.
Only @xetorthio and me talked our opinions and it's "remove ShardedJedis at 3.0.0"
(I'd love to see @marcosnils on google groups. @xetorthio would wish to discuss issues or opinions from google groups, but I haven't seen.)

But we cannot rely on google groups for gathering opinions about issue.
Please see google groups post. How much replies on each post?

tl;dr : We should make a decision by our hands though we cannot gather opinions or thoughts.
And we were agreed to remove ShardedJedis at 3.0.0 from google groups post.

@sitz
Copy link

sitz commented May 26, 2014

@HeartSaVioR Yes, Indeed. I was checking out Predis implementation for Client side sharding implementation and instead of just allowing a hashing algorithm (unlike Jedis) they allow entire distribution strategy to be customized.
Maybe, ShardedJedis can be forwarded in that direction so that one can write entire customization strategy and have control on things rather than deprecating it?

@allen-servedio
Copy link

As SITZ said, I think there is value in allowing custom versions of ShardedJedis so that users could add what makes sense for their application. Consistent hashing is one example (esp. for using Redis as a cache).

@xetorthio
Copy link
Contributor

Right now you can write your own hashing algo and use it with jedis. So it
is supported.
I can see how ShardedJedis can still be useful even with JedisCluster.
Maybe we should take this discussion to redis group and get the opinion of
@antirez and others
On Jun 19, 2014 10:40 PM, "allen-servedio" notifications@github.com wrote:

As SITZ said, I think there is value in allowing custom versions of
ShardedJedis so that users could add what makes sense for their
application. Consistent hashing is one example (esp. for using Redis as a
cache).


Reply to this email directly or view it on GitHub
#629 (comment).

@HeartSaVioR
Copy link
Contributor Author

Another one approach - we can simplify ShardedJedis to only support Consistent
Hashing.
Can ShardedJedis remove dead nodes and add additional nodes online(after
initialized)? It would need to implement Consistent Hashing.

2014년 6월 23일 월요일, Jonathan Leibiuskynotifications@github.com님이 작성한 메시지:

Right now you can write your own hashing algo and use it with jedis. So it
is supported.
I can see how ShardedJedis can still be useful even with JedisCluster.
Maybe we should take this discussion to redis group and get the opinion of
@antirez and others
On Jun 19, 2014 10:40 PM, "allen-servedio" <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

As SITZ said, I think there is value in allowing custom versions of
ShardedJedis so that users could add what makes sense for their
application. Consistent hashing is one example (esp. for using Redis as
a
cache).


Reply to this email directly or view it on GitHub
#629 (comment).


Reply to this email directly or view it on GitHub
#629 (comment).

Name : 임 정택
Blog : http://www.heartsavior.net / http://dev.heartsavior.net
Twitter : http://twitter.com/heartsavior
LinkedIn : http://www.linkedin.com/in/heartsavior

@drewmorris
Copy link

I do not understand why we would drop support for ShardedJedis in favor of JedisCluster when they seem to have different use cases. JedisCluster seems to depend on Redis Cluster and therefore it is locked into the master/slave approach that Redis Cluster provides. ShardedJedis allows for application-level control over sharding and, if combined with the ability to check if a Redis node is up/down, could be used to provide a resilient system that re-allocates shard keys across available nodes if one node goes down.

I would advocate that shardedJedis provides application developers to choose their own models for managing and distributing cache in a much finer-grained way than the generic Redis Cluster model which relies on master-slave. Has this issue been fully decided or is there a chance that the ShardedJedis functionality can be saved since it does not impact the use of ClusteredJedis but provides more functionality and flexibility.

@antirez
Copy link

antirez commented May 14, 2015

For what is worth, I believe that for many plan get/set caching setups where writes are idempotent and no data structures are used, memcached-alike consistent hashing targeting available servers automatically is a very viable option compared to Redis Cluster. So I would retain the code inside Jedis. Btw the goal for the future is to have direct support for something like that in Redis Cluster itself.

@drewmorris
Copy link

So perhaps it makes sense to support (and maybe even augment) ShardedJedis in the near future until Redis Cluster matures to be production-ready and to support a more complete set of functionality relating to these scenarios.

@antirez
Copy link

antirez commented May 14, 2015

@drewmorris Redis Cluster is production ready now, and can be used for caching scenarios (and should be used every time the cache uses data structures, otherwise consistent hashing will do a mess). Here the problem is that Redis Cluster has not a special mode of operation for volatile caching where availability of the cache is the most important thing, no failover is needed, consistency requirements are weak since writes are idempotent and TTL/LRU are enough to evict old garbage, and so forth. Most of the times I would use Redis Cluster, but not when there are specific set of requirements, so why to delete the code from Jedis?

@drewmorris
Copy link

@antirez I completely agree with you. I think there are definitely scenarios where ShardedJedis could be needed and I think it warrants keeping the code. On the other point I wasn't aware that Redis Cluster was production ready for all scenarios yet. Am I correct that it requires a master-slave scenario though and that it doesn't support a mode where all nodes can be used without slaves and where node failure results in new read/writes to a specific hash range being re-distributed across the remaining nodes?

@xetorthio
Copy link
Contributor

Yes. There are valid arguments here to keep supporting it.

On Thu, May 14, 2015 at 10:20 AM drewmorris notifications@github.com
wrote:

@antirez https://github.com/antirez I completely agree with you. I
think there are definitely scenarios where ShardedJedis could be needed and
I think it warrants keeping the code. On the other point I wasn't aware
that Redis Cluster was production ready for all scenarios yet. Am I correct
that it requires a master-slave scenario though and that it doesn't support
a mode where all nodes can be used without slaves and where node failure
results in new read/writes to a specific hash range being re-distributed
across the remaining nodes?


Reply to this email directly or view it on GitHub
#629 (comment).

@antirez
Copy link

antirez commented May 14, 2015

@drewmorris yes you are right it's master-slave design. Failures are not handled like with consistent hashing as you said, but via master-slave failover and replicas migration when masters remain orphaned, this is needed in order to have certain degrees of consistency. When you use consistent hashing with Redis, if key foo is a list A,B,C moving it to a different server to handle failures means that a new push operation on the list will get you a version of foo which may contain only the new elements, then the failure is resolved and the node is back online and you have back A,B,C. In a workload based on idempotent writes, this may not be an huge issue (however note that if a previous server returns available it is very easy to get tons of stale keys), but it is if keys have data structures as values. Depends on the use case basically...

A Redis Cluster mode specific for caching in order to better cover use cases where now consistent hashing is used would be still different than consistent hashing, even if only composed of master nodes. For example it could work like that: when a master is not available instead of triggering a failover to a slave (that does not exist) the cluster would reassign hash slots of this master to other available masters in order for the computation to continue. When the old master is back online, we could at least avoid to insert it back into the cluster without any clue like it happens with a naive consistent hashing implementation. We could at least:

  1. Reassign back the slots to the old master but clearing them from all the data, so the application will populate them back.
  2. Or even try to MIGRATE the new keys versions into the old master, possibly with some data-type specific merge function.

Or something else... but because Redis Cluster has more state we can do a better job than client side consistent hashing IMHO.

@HeartSaVioR
Copy link
Contributor Author

At first, we decided to stop supporting ShardedJedis cause we expect Redis Cluster can replace it.
But if you read whole comments here, you can know that I already know Redis Cluster cannot replace ShardedJedis at least now.

But now we have other reason to stop, cause of maintaining cost.
In Jedis 3.0.0 we're preparing to introduce full support of Redis Cluster (single key, multiple keys, binary, lua script).
Here're related PR, #671 and #687. Though it was constucted on basic implementation of JedisCluster, but it requires so many works, too.
It adds about 4000+ lines of code, and its interfaces are not fully compatible with current Jedis because of limitation or design concept of Redis Cluster.
I think we definitely need to drop functionality to reduce maintaining cost, and ShardedJedis seems to best fit since it's unique feature compare to Redis so dropping it doesn't break Jedis' objective.

Here's what I remove ShardedJedis and relevant interfaces from Jedis, #875.

  • We can remove about 3000 lines of code.
  • We're no longer having to separate single key and multi key operations (Oh, except JedisCluster.) so our interfaces can be simplified.
  • We're no longer missing to add new commends from ShardedJedis.

It doesn't mean we deprecate ShardedJedis. We were decided to maintain ShardedJedis at 2.x, and remove ShardedJedis at 3.x. But that can be changed. Who knows.

@antirez
Copy link

antirez commented May 14, 2015

@HeartSaVioR my guess is that SharedJedis should not be part of Jedis itself, but just a separated library using Jedis. Then it can have a different set of developers and a different life: if it is still interesting enough, it will survive, if everybody will switch to Redis Cluster, it will likely die. IMHO to be more useful it should get more complex, and it is possible to add complexity once extrapolated away from Jedis. The additional complexity could allow to do more interesting things like putting wallclock timestamps in keys and do read repair or alike, basically it should be a multi master AP system with some decent feature in my vision...

@antirez
Copy link

antirez commented May 14, 2015

Another thing that should be considered is if those features are better handled by a proxy like Twemproxy instead of being copied again and again in the context of each client library. The obvious reply is YES but I don't see enough work in this area...

@HeartSaVioR
Copy link
Contributor Author

@antirez
Yes. Actually ShardedJedis becomes complex cause we're trying to maintain interfaces naturally (I mean, separation of single key operations)
If we (or others) are providing consistent hash which returns Jedis instance when single key is provided instead of maintaining ShardedJedis, issue may be simplified. That way users should keep in mind he/she shouldn't call operation with another key(s).

@HeartSaVioR
Copy link
Contributor Author

Please keep in mind that it is very hard to maintain consistent hash via client side since clients are hard to check added/removed/killed nodes, as @antirez stated.

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.

7 participants