JedisClusterConnectionHandler.initializeSlotsCache can result in two entries for a single redis instance

[roblg]

Expected behavior

It seems like JedisCluster should always have exactly one Jedis instance per redis server. It should compute its lists of nodes from cluster slots (or cluster nodes -- I forget which one is recommended now), and it shouldn't add the start nodes, since they should already be in the list anyway.

(i.e., I think the behavior would be correct if these lines were removed from JedisClusterConnectionHandler line ~50:

    for (HostAndPort node : startNodes) {
      cache.setNodeIfNotExist(node);
    }

Actual behavior

JedisClusterConnectionHandler.initializeSlotsCache initializes the slots cache by querying the redis server, and also adds whatever startNodes it was given. If the start nodes were given as hostnames, this will result in two entries for each start node: one for the host name, and one for the IP address reported by cluster slots. Queries against the cluster continue to work, but iterating over the nodes in the cluster (e.g., for aggregating per-node stats) also includes these bogus entries.

Steps to reproduce:

Our setup:

1. Two redis instances running on separate machines, named redis-1 and redis-2
2. Java application running on a third machine: jobs-1. Specifies cluster start nodes as redis-1:6379, redis-2:6379.
3. Iterate through pool nodes with:

for (JedisPool pool : cluster.getClusterNodes().values()) {
    try (Jedis jedis = pool.getResource()) {
        // do stuff
    } catch (Exception e) {
        // we hit this exception any time `pool.getResource()` returns one of the "phantom" nodes w/ hostname instead of IP 
    }
}

We have a workaround that allows us to specify per-machine hostnames (we just us InetAddress to do a DNS lookup of the IP before we send it to jedis), BUT it would be really nice to be able to pass the host name of a load-balancer pool to Jedis for service discovery. It could make an initial query to that machine, learn all the node info from cluster slots, and that way we wouldn't need any hard-coded IP address or machine names in our configuration.

If this seems like a reasonable approach, LMK and I'll attempt a PR.

Thanks!

Redis / Jedis Configuration

Jedis version:

2.8.0

Redis version:

3.0.x

Java version:

8.x

[marcosnils]

@roblg thanks for reporting this issue. I see the situation you're facing and I totally agree that it's something that we need solve from the Jedis perspective.

I think I don't understand the approach you're suggesting to pass a host for a load-balancer pool for Jedis to do service discovery. What do you mean by a load-balancer pool?, how is Jedis supposed to query the nodes from this load-balanacer pool?. Which would be the API to do that?

I'm thinking about two possible solutions for this ATM:

1. Use the startnodes just to query the cluster and don't add them to the current internal node list
2. Make a DNS resolution to get the startnodes IP in order to standardize.

From the two I think I prefer the 1st option as it doesn't involve DNS resolutions (which might lead to possible errors) and it's less error prone.

Thoughts?

[roblg]

@marcosnils -- solution 1 is in line with what I was thinking. That would solve the issue for our use case.

I think I don't understand the approach you're suggesting to pass a host for a load-balancer pool.

In our datacenter, we'd like to not have every Java application need to know the hostname/IP address of a redis instance, so that we can add/remove nodes whenever we want without needing to update our jedis configuration somewhere with a new hostname. Instead, we'd like to create a virtual pool of machines, named e.g. redis-pool, and connect just once on startup to any node in that pool and issue the cluster slots command to learn about all the machines in the cluster. If we implement solution 1 above, we get this behavior for free. We can just point jedis to redis-pool as a start node, and then jedis can discover all the machines in the cluster. It just keeps things a little cleaner for our configuration.

Thanks!

[HeartSaVioR]

Closed by #1240