Renew cluster slots strategy update

[yangbodong22011]

This PR solves #2551, a problem solving scenario is as follows:

1. JedisCluster jc = new JedisCluster(vip); // now vip -> ip1
2. change vip -> ip2
3. renewClusterSlots just use getShuffledNodesPool, that is ip1
4. connect to ip1, and will never connect to ip2

The new strategy is as follows:

1. First, if jedis(meet JedisRedirectionException) is available, use jedis renew.
2. Then, we use startNodes to try, as long as startNodes is available,
   whether it is vip, domain, or physical ip, it will succeed.
3. Finally, we go back to the ShuffledNodesPool and try the remaining physical nodes.

[sazzad16]

@yangbodong22011 Not only this is a breaking change but also backward incompatible.

• (Preferable) Could you find a non-breaking solution?
• If not, at least find a backward compatible solution.
  • Hint: We can handle that startNodes can be null.

You are completely disregarding getShuffledNodesPool(). IMO we should check those first and if no solution is found, then check among startNodes.

[yangbodong22011]

• (Preferable) Could you find a non-breaking solution?

• If not, at least find a backward compatible solution.

  • Hint: We can handle that startNodes can be null.

This is not actually a behavior visible to the user, so I think we can modify it without having to be compatible with the old (and may cause errors) logic.

If it should be compatible, maybe we can add a refreshConfigByStartNode option to control the two logics.

I guess the reason you think it should be compatible is that the performance of the getShuffledNodesPool() method will be higher, because operations such as new connections and auth are avoided.

You are completely disregarding getShuffledNodesPool(). IMO we should check those first and if no solution is found, then check among startNodes.

This may be a bit difficult, because we did not distinguish whether the type in NodesPool is MASTER or SLAVE, (we think that MASTER will always belong to the cluster), but there are two solutions:

• After taking out the Jedis connection, call jedis.clusterNodes() to get your own type (master or slave)
• When we add to the NodesPool, use the key to record the type, such as nodes.put(127.0.0.1:6379:master, nodePool);

[sazzad16]

This is not actually a behavior visible to the user, so I think we can modify it without having to be compatible with the old (and may cause errors) logic.

Practically, yes. But theoretically no; because JedisClusterInfoCache is a public class.

I guess the reason you think it should be compatible is that ...

No. If the behavior is changed significantly, we may have to push it for 4.0 instead of 3.x.

[sazzad16]

@yangbodong22011 WDYT about #2644 ?

[yangbodong22011]

@sazzad16 @dengliming PING, pls have a look when you have time.
also updated top comment.

[sazzad16]

About existing JedisClusterInfoCache constructors, we have to keep those. So, keep these constructors, deprecate these, forward to new constructors (like this(..., ..., (Set<HostAndPort>) null); and handle startNodes == null in other methods.

[yangbodong22011]

About existing JedisClusterInfoCache constructors, we have to keep those. So, keep these constructors, deprecate these, forward to new constructors (like this(..., ..., (Set<HostAndPort>) null); and handle startNodes == null in other methods.

done

[sazzad16]

LGTM so far.

[codecov-commenter]

Codecov Report

Merging #2643 (7f5efba) into master (023b362) will decrease coverage by 0.00%.
The diff coverage is 64.28%.

❗ Current head 7f5efba differs from pull request most recent head 873562d. Consider uploading reports for the commit 873562d to get more accurate results

@@             Coverage Diff              @@
##             master    #2643      +/-   ##
============================================
- Coverage     55.03%   55.03%   -0.01%     
- Complexity     3154     3155       +1     
============================================
  Files           195      195              
  Lines         12069    12086      +17     
  Branches        689      690       +1     
============================================
+ Hits           6642     6651       +9     
- Misses         5200     5206       +6     
- Partials        227      229       +2     

Impacted Files	Coverage Δ
...rc/main/java/redis/clients/jedis/JedisCluster.java	23.94% <0.00%> (ø)
...ava/redis/clients/jedis/JedisClusterInfoCache.java	75.15% <56.25%> (-4.30%)	⬇️
src/main/java/redis/clients/jedis/Jedis.java	84.84% <100.00%> (-0.01%)	⬇️
src/main/java/redis/clients/jedis/Protocol.java	87.89% <100.00%> (ø)
...nts/jedis/providers/ClusterConnectionProvider.java	78.18% <100.00%> (+3.63%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8476aaf...873562d. Read the comment docs.

[dengliming]

LGTM

[yangbodong22011]

@sazzad16 I would like to backport this PR to version 3.9.x, what do you think?

[sazzad16]

@yangbodong22011 I may consider it for 3.10.0

[yangbodong22011]

@yangbodong22011 I may consider it for 3.10.0

That's ok, what should I do next? Submit a PR to the 3.x branch?

[sazzad16]

Submit a PR to the 3.x branch?

That'd be helpful.

[yangbodong22011]

@sazzad16 #3370