Attempted fix for #1229 - multiple entries in node list for same host #1239
Conversation
Revert "Add test for JedistCluster's method blpop" This reverts commit f705810.
Fix blpop and brpop in JedisCluster
* Fixes redis#896 * Renaming a key to itself is now essentially a noop
…ior-of-rename Follow up new behavior of rename from test
Deprecates psetex(String, int, String) in favour of psetex(String, long, String).
PSETEX should take a long parameter
Removes deprecated psetex(String, int, String).
…aram Remove deprecated PSETEX with int parameter
…-from-jediscluster Deprecate BasicCommands from JedisCluster since it has no meaning
…scan-in-jediscluster Fix: slot key is missed from *scan in JedisCluster
…-cluster deprecating blpop and brpop that will be removed in 3.0.0
* checks that Jedis marks broken status properly while running scriptExists
…reply-does-not-mark-broken fix bug: getIntegerMultiBulkReply() doesn't handle broken status
To improve usability we're changing the visibility of `returnResoure` and `returnBrokenResource` in favor of `jedis.close()` which automatically returns the resource to the pool accordingly. As `Pool` and `Jedis` are currently in different packages i've created a JedisPoolAbstract class to provide a bridge between the two implementations
Bump commons-pool2 to latest version
Fixes JedisCluster.psubscribe() to call the correct psubscribe method.
Add support to *scan parameters to ShardedJedis and JedisCluster
Don't return JedisConnectionException if pool is exahusted
|
@roblg thanks for submitting a patch. I believe it's heading in the proper direction. In order to test this you might try to setup JedisCluster like this: Set<HostAndPort> jedisClusterNode = new HashSet<HostAndPort>();
jedisClusterNode.add(new HostAndPort("localhost", 7379));
jedisClusterNode.add(new HostAndPort("some_other_host", 7379));
JedisCluster jc = new JedisCluster(jedisClusterNode);
// Assert on jc.getClusterNodes().size()With the previous code JedisCluster will populate all the hosts in the cluster plus "some_other_host" (even though it's invalid). Your test needs to assert that supplied hosts are not added into the node list. |
roblg
force-pushed
the
no-seed-nodes-in-node-list
branch
from
fb48f8a to
00ad991
Compare
|
@marcosnils good call. I realized while I was writing the "invalid host" test that I was mistaken about how the 127 vs. localhost equality was happening. I actually got the "broken" behavior if I created a PR updated. |
|
@roblg LGTM. I'd like @HeartSaVioR or @xetorthio to review before merging. I'd also like to schedule this for 2.9 as it changes the way JedisCluster is initializing. Even though I believe it "shouldn't" cause any problems, I propose to move it to a minor release just in case |
|
@roblg |
|
@HeartSaVioR @marcosnils would you like me to update the PR to be based on master? |
|
@roblg yes please. We'll take care to downmerge to 2.9 |
|
Yes, if you don't mind. Thanks in advance! |
|
@marcosnils You're really fast! lol |
|
@HeartSaVioR 👅 going to bed. Can you please merge once changes are made?. Thx!. |
roblg
force-pushed
the
no-seed-nodes-in-node-list
branch
from
00ad991 to
7366fc1
Compare
|
@roblg Ah, one thing we didn't say... PR should be re-created against master. It's a limitation of Github PR. |
|
@HeartSaVioR yeah that kind of surprised me. I've been using Bitbucket at work for several years. Got it sorted out though (I hope). |
26 participants
Hey all, this is my attempt at a fix for #1229. It sounded like the approach seemed reasonable, so I thought I'd take a stab at it.
Tests pass for me locally against the unstable branch of redis. I tried to add a new test for my specific case where equality check in HostAndPort fails, so multiple nodes end up in the list, but there's already code to convert
127.0.0.1-->localhostinHostAndPort, so it wasn't clear how I'd be able to test my particular situation safely in many different test environments.I based my fix on the 2.8 branch, but I can switch it to the 2.9 or master branches if that makes more sense.
Thanks for considering!