Small fix in JedisClusterInfoCache.discoverClusterNodesAndSlots.

[adiamzn]

Description:

The change calls Jedis.cluserSlots before reset is called instead of the other way around.

Why the change?

the reset function deletes the nodes and slots fields.
If the Jedis.clusterSlots throws an exception in the try-catch block
the function fails and the data is deleted.
It would be nicer if the data (nodes and slots) were left untouched if jedis.clusterSlots throws an exception.

[nbraun-amazon]

Approved

[yangbodong22011]

we just call discoverClusterNodesAndSlots at 1:

private void initializeSlotsCache(Set<HostAndPort> startNodes, JedisClientConfig clientConfig) {
  ArrayList<HostAndPort> startNodeList = new ArrayList<>(startNodes);
  Collections.shuffle(startNodeList);
    for (HostAndPort hostAndPort: startNodeList) {
    try (Jedis jedis = new Jedis(hostAndPort, clientConfig)) {
      cache.discoverClusterNodesAndSlots(jedis); // 1
      return; // 2
    } catch (JedisConnectionException e) {
      // try next nodes
    }
  }
}


  public void discoverClusterNodesAndSlots(Jedis jedis) {
    w.lock();

    try {
      reset(); // 3
      List<Object> slots = jedis.clusterSlots(); // 4
      ...

When jedis.clusterSlots() does not throw an exception, the return at 2 will be executed. On the contrary, if an exception is thrown, enter discoverClusterNodesAndSlots again, it does not matter to execute 3 or 4 first, because at this time the variables of nodes and slots are empty (they were not filled last time, because jedis.clusterSlots() throws an exception NS)

I would like to ask, does this PR fix the actual problems encountered in the production environment, or is the idea obtained by reading the code?

[adiamzn]

Hi Yang,
Agree that this does not cause any issues the way the function is currently called.
As you mentioned this function is currently only called from [1] and this is fine.

Since the function discoverClusterNodesandSlots is part of the API for the class JedisClusterInfoCache, I think it is a good idea to add a better safety guarantee.
The idea came up just from reading the code, not from prod environment
All the best,
Adi

[yangbodong22011]

List<Object> slots = jedis.clusterSlots();
w.lock();

[yangbodong22011]

Since the function discoverClusterNodesandSlots is part of the API for the class JedisClusterInfoCache, I think it is a good idea to add a better safety guarantee.

@adiamzn Agree, I think we can move jedis.clusterSlots() to the top of w.lock(), just like the discoverClusterSlots function.

[adiamzn]

agreed, made the change

[yangbodong22011]

@adiamzn jedis.clusterSlots() to the top of w.lock(), the lock time can be shortened.

[adiamzn]

true, fixing