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

RedisCluster that can use ReadOnly slaves #1789

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

digitalsonic
Copy link

JedisCluster uses only Master nodes in the cluster, all the Slave nodes are only standby nodes. In many situation this is a waste of resources.

So I write an EnhancedJedisClusterInfoCache besides the original JedisClusterInfoCache.
When we want to read only from Master, JedisCluster will use JedisClusterInfoCache, nearly nothing changed. But if we want to read from Slave or both Master and Slave nodes, JedisClusterConnectionHandler will create an EnhancedJedisClusterInfoCache.

In the new cache class, there are two maps, one for master nodes and the other for slave nodes.
All the Jedis instances in the slave pool have been sent a ReadOnly command when they were created.

When renewing the slot cache (master goes down or slot migration), both of the two maps will be reset, the JedisPool will be destroyed. So we don't need to worry about writing to a ReadOnly node.

Thanks for reviewing my codes.

P.S.
I've read #790 #830 and some other posts and issues about ReadOnly slaves. And my thought is definitely same with @marcosnils mentioned in #1522 .

Add a readFrom parameter, whose value can be MASTER(default), SLAVE and
BOTH.
This parameter will be passed to all the read command, such as GET,
MGET etc.
When the JedisPool of a slave node is created, READONLY will be sent
immediately.
JedisClusterInfoCache has a lot of changes!
The JedisClusterInfoCache has been reverted to the original one.
Add a new AbstractJedisCluserInfoCache.
If the user choose to use ReadFrom.MASTER, the old
JedisClusterInfoCache will be created.
If the user choose ReadFrom.SLAVE or BOTH, the
EnhancedJedisClusterInfoCache will be created.
EnhancedJedisClusterInfoCache can remove JedisPools in the process of
nodes setup
@phenomax
Copy link

Got all code up & running. Looking forward to this getting merged!

@digitalsonic
Copy link
Author

Can anybody else review this PR?
@marcosnils @sazzad16 can you help?
Thank you.

@venkatrangan
Copy link

@digitalsonic when is this going to be merged?

@digitalsonic
Copy link
Author

@venkatrangan sorry, i don't know, either.

@darren-fu
Copy link

great pr!when it can be merged and release!

@angelim
Copy link

angelim commented Aug 2, 2019

Any plans on merging this?

@chandresh-pancholi
Copy link

Any plan for merging this?

2 similar comments
@Kate-liu
Copy link

Any plan for merging this?

@OneCodeMonkey
Copy link

Any plan for merging this?

@sazzad16
Copy link
Collaborator

To everyone who have asked about merging this PR, first of all I thank you all for your interest in Jedis.

This PR has a few backward incompatible parts which wouldn't allow a smooth transition from one version to another for the users who have codes based on the concerned classes. So this specific PR is unlikely to be merged.

We have kept open this PR hoping that someone would pick-up the idea and craft another PR with not so backward incompatible parts or at least with "salvageable" ones.

Thanks again.

@eugeniyk
Copy link

@sazzad16 can you please elaborate more on backward-incompatible changes? I've spotted only JedisClusterConnectionHandler. Since this was introduced before v4.x, is there an existing way already to be able to read from replicas in readonly mode?

Btw looks like by PR JedisCluster / BinaryJedisCluster / JedisClusterConnectionHandler / JedisFactory require using builder pattern for extendibility, which will require.. a breaking change

@esigma5
Copy link

esigma5 commented May 9, 2023

@sazzad16 can you please elaborate more on backward-incompatible changes? That way I could try to offer a new PR with less breaking changes.
Thanks!

@sazzad16
Copy link
Collaborator

@esigma5 @eugeniyk Just try not to remove too many public methods and public classes and/or change their behavior with current parameters.

PS: We have started publishing alpha releases for Jedis 5.0. Now is a very good time to work on things that may have breaking parts. Also, if you have something promising please let us know, preferably as WIP or POC, in case we want to wait.

@eugeniyk Sincere apologies I missed your comment above.

@shiranyosef1
Copy link

any news? waiting for it

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.

None yet