Issue #1252: Fix creating a lot of new Jedis instances on unstable cluster, fix slots clearing without filling #1253

Spikhalskiy · 2016-04-06T13:07:44Z

I started to investigate Issue #1252 and PING-PONG counts especially.

The most interesting things I found in stack traces on our runtime:

    at redis.clients.jedis.BinaryJedis.ping(BinaryJedis.java:106)
    at redis.clients.jedis.JedisSlotBasedConnectionHandler.getConnection(JedisSlotBasedConnectionHandler.java:41)
    at redis.clients.jedis.JedisSlotBasedConnectionHandler.getConnectionFromSlot(JedisSlotBasedConnectionHandler.java:64)
    at redis.clients.jedis.JedisClusterCommand.runWithRetries(JedisClusterCommand.java:115)
    at redis.clients.jedis.JedisClusterCommand.run(JedisClusterCommand.java:30)
    at redis.clients.jedis.JedisCluster.setex(JedisCluster.java:292)

Looks absolutely abnormal - we have nothing for slot and fallback to new Jedis to random node.

Here you can find my changes and fixes for place, that could cause this issue + some refactoring, remove of some duplicate map lookups/keys building.

System behavior before and after applying this changes:

Spikhalskiy · 2016-04-06T13:12:00Z

src/main/java/redis/clients/jedis/JedisClusterInfoCache.java

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

+        // We should clear slots after getting result about cluster slots, because if we got exception or timeout in


most important part here

At first, when we fix JedisSlotBasedConnectionHandler#getConnectionFromSlot, this comment becomes invalid.

And IMO, clearing slots first or later is up to amount of slot changeset. For example, if most of slots are changed, it would be better to try discovering slots again instead of relying on outdated slot information, which results in MOVED again. If only a little of slots are changed, we should take latter. It would be not a big deal cause we don't do random connect later.

JedisSlotBasedConnectionHandler#getConnectionFromSlot, this comment becomes invalid.

Not completely.

First thread started to discover. Clear nodes. Release lock while going to next node.

On this time second thread went to JedisSlotBasedConnectionHandler#getConnectionFromSlot, very possible that we could just use old pool, but we see empty collection and connect to discovering party.
I would prefer to don't publish empty collection while we can leave old one.

And IMHO, clearing slots first or later is up to amount of slot changeset

I'm not sure if I got you. We clear it completely anyways. I just moved clearing after external code. So, to avoid situation of exception in external code, which leads to publishing empty slots collection.

And IMHO, clearing slots first or later is up to amount of slot changeset

I'm not sure if I got you. We clear it completely anyways. I just moved clearing after external code. So, to avoid situation of exception in external code, which leads to publishing empty slots collection.

Imagine first trial of clusterSlots() failed (so lock is released), and next one calls getConnectionFromSlot(). If we don't clear slots, it will get Jedis instance from cache which could be invalidated (cause we fails to update so doesn't catch up latest information) or not (node for slot is not changed).

Former will try to request wrong node, and receive MOVED, which also initiate slot cache update. If we clear cache first, it doesn't need to request but just update slot cache immediately.

@HeartSaVioR Got you. And I'm not sure what is better... It could get right node and don't stuck at all. In worst case - we will get same result with rediscovering call. I would prefer to give a chance and think that more slots is still on place. But if you think that it's better to publish clear state - can revert this part.

Maybe we want to hold a lock during trying all nodes.
I see what you said, we release the write lock before completing slot cache update so there's race again.

It should be more stable behavior and makes sense. I will take a look how to implement it.

Spikhalskiy · 2016-04-06T13:19:15Z

I'm going to put it to prod load today, will let you know if it helps.

Spikhalskiy · 2016-04-06T15:26:06Z

@HeartSaVioR reworked with one lock for trying all nodes; will squash before merging

marcosnils · 2016-04-06T15:29:02Z

I'll check this and #1252 later today.

HeartSaVioR · 2016-04-06T15:59:13Z

@Spikhalskiy Github introduces new feature - 'squash before merging' so all you need to consider is rebasing to catch up with master.
I'll review later today.

Spikhalskiy · 2016-04-06T16:02:30Z

I don't like this feature, because it merges without merge commits :) So, it's impossible to continue branch and merge once more. But you merge in multiple branches - so it's cherry-pick in any case.

… cluster, fix slots clearing without filling

…iscover cluster

Spikhalskiy · 2016-04-06T17:34:36Z

This helped. Got rid of this type of error flow, things became much better. Waiting it in the upstream for further improvements.

System behavior before and after applying this change:

But found one more broken flow. After finalizing and merge this PR will start to think about implementation.

Shortly (this trace based on this code state):

    at redis.clients.jedis.BinaryJedis.ping(BinaryJedis.java:106)
    at redis.clients.jedis.JedisSlotBasedConnectionHandler.getConnection(JedisSlotBasedConnectionHandler.java:41)
    at redis.clients.jedis.JedisClusterCommand.runWithRetries(JedisClusterCommand.java:113)
    at redis.clients.jedis.JedisClusterCommand.runWithRetries(JedisClusterCommand.java:131)
    at redis.clients.jedis.JedisClusterCommand.run(JedisClusterCommand.java:30)
    at redis.clients.jedis.JedisCluster.setex(JedisCluster.java:292)

We get timeout. After that we decide to try random node 0_o. It's already strange. After that we get random Jedis with PING-PONG. On this random node we, of course, get MOVED and... start cluster rediscovering. And all of this with just one timeout.

This could be fixed:

Why do we try random node on timeout?
Why with Ping-Pong? No reason to don't believe Jedis from random pool here. It's waste of time and additional load.
Why start rediscovering if you tried random node intentionally and of course didn't find key there?

All of this looks suspicious and could be fixed. Suggestions where to start?

Spikhalskiy · 2016-04-07T13:39:47Z

Could we finalize it today to make further fixing easier?

HeartSaVioR · 2016-04-07T15:06:59Z

@Spikhalskiy Even better. LGTM.
@marcosnils Could you review new changeset?

HeartSaVioR · 2016-04-07T15:21:16Z

Please note that I didn't look deeply at your last comment. Sorry I don't have time to concentrate. I'll try to have a look and comment later.

Spikhalskiy · 2016-04-07T15:31:37Z

@HeartSaVioR Thank you! Sure, take your time. When I take a look deeply at code and possible solutions I will share my suggestions too in separate PR.

marcosnils · 2016-04-07T15:33:33Z

@Spikhalskiy @HeartSaVioR I'll take care of this today after work.

amazing job!.

Spikhalskiy · 2016-04-20T16:47:54Z

@HeartSaVioR @marcosnils Any updates here?

xetorthio · 2016-07-08T11:37:24Z

src/main/java/redis/clients/jedis/JedisClusterInfoCache.java

+    for (Object slotInfoObj : slots) {
+      List<Object> slotInfo = (List<Object>) slotInfoObj;
+
+      if (slotInfo.size() <= MASTER_NODE_INDEX) {


Why are we doing this? Masters could not have replicas, right?

It's old code if (slotInfo.size() <= 2) {, which is absolutely not related to PR. You can ask original author.

xetorthio · 2016-07-08T11:42:59Z

Checked your code and I understand it, but I have hard time to relate it to the issue because of all the refactoring. Would you mind describing briefly what this code is fixing?

Spikhalskiy · 2016-07-08T12:46:01Z

@xetorthio For your understanding, I separated PR to two commits for review. The main part is the first commit: I underline 2 main places there by comments (#1253 (comment) and #1253 (comment)).
Second commit is refactoring and moving the lock to the top level suggested here #1253 (comment)

The most critical here shortly - if we have nothing alive for the slot - we just rediscover cluster state. Not trying to establish a random connection to a random node of the cluster with predicted MOVED after that and same rediscovering, but only after that. It's what I remember.

xetorthio · 2016-07-08T14:02:59Z

Got you! After your clarifications and checking again the code, _I think it is OK to merge this_.

I would make further changes down the road since the code (not yours) is hard to understand. Mainly clusterSlots command return is extremely ugly, making it super hard to use and even worse to under the code.

Another suggestion for future improvements is to do the cluster slot renewal in a background thread, which should make the locking unnecessary.

Created issues to discuss this: #1346 and #1347

…uster, fix slots clearing without filling (#1253) * Issue #1252: Fix creating lot of new Jedis instances on unstable cluster, fix slots clearing without filling * Issue #1252: Acquire one long lock for trying all nodes when rediscover cluster Conflicts: src/main/java/redis/clients/jedis/JedisClusterInfoCache.java

marcosnils · 2016-07-19T13:14:42Z

Downmerged to 2.8 and 2.9 respectively. Thx again everyone involved to make this happen.

Spikhalskiy reviewed Apr 6, 2016
View reviewed changes

Spikhalskiy changed the title ~~Issue #1252: Fix creating lot of new Jedis instances on unstable cluster, fix slots clearing without filling~~ Issue #1252: Fix creating a lot of new Jedis instances on unstable cluster, fix slots clearing without filling Apr 6, 2016

Spikhalskiy force-pushed the issue-1252-1 branch 3 times, most recently from d9c7d52 to b24cb3a Compare April 6, 2016 15:25

Issue redis#1252: Fix creating lot of new Jedis instances on unstable…

a5961d0

… cluster, fix slots clearing without filling

Spikhalskiy force-pushed the issue-1252-1 branch from b24cb3a to 0635acf Compare April 6, 2016 16:12

Issue redis#1252: Acquire one long lock for trying all nodes when red…

ec87a52

…iscover cluster

Spikhalskiy force-pushed the issue-1252-1 branch from 0635acf to ec87a52 Compare April 6, 2016 16:40

HeartSaVioR added the wait for more reviews label Apr 7, 2016

HeartSaVioR added this to the 2.8.2 milestone Apr 7, 2016

Spikhalskiy mentioned this pull request Apr 13, 2016

Issue #1252: Random node + rediscovery on connection exception replaced with rediscovery at the end #1256

Merged

marcosnils mentioned this pull request Apr 25, 2016

Snowball effect with reconnecting to poor performing node #1252

Closed

xetorthio reviewed Jul 8, 2016
View reviewed changes

marcosnils merged commit 69d4080 into redis:master Jul 11, 2016

marcosnils added ready to merge backport required and removed wait for more reviews ready to merge labels Jul 11, 2016

marcosnils removed the backport required label Jul 19, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #1252: Fix creating a lot of new Jedis instances on unstable cluster, fix slots clearing without filling #1253

Issue #1252: Fix creating a lot of new Jedis instances on unstable cluster, fix slots clearing without filling #1253

Spikhalskiy commented Apr 6, 2016

Spikhalskiy Apr 6, 2016

HeartSaVioR Apr 6, 2016

Spikhalskiy Apr 6, 2016

HeartSaVioR Apr 6, 2016

Spikhalskiy Apr 6, 2016

HeartSaVioR Apr 6, 2016

Spikhalskiy Apr 6, 2016

Spikhalskiy commented Apr 6, 2016

Spikhalskiy commented Apr 6, 2016

marcosnils commented Apr 6, 2016

HeartSaVioR commented Apr 6, 2016

Spikhalskiy commented Apr 6, 2016

Spikhalskiy commented Apr 6, 2016 •

edited

Loading

Spikhalskiy commented Apr 7, 2016

HeartSaVioR commented Apr 7, 2016

HeartSaVioR commented Apr 7, 2016

Spikhalskiy commented Apr 7, 2016

marcosnils commented Apr 7, 2016

Spikhalskiy commented Apr 20, 2016

xetorthio Jul 8, 2016

Spikhalskiy Jul 8, 2016 •

edited

Loading

xetorthio commented Jul 8, 2016

Spikhalskiy commented Jul 8, 2016 •

edited

Loading

xetorthio commented Jul 8, 2016

marcosnils commented Jul 19, 2016

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

		// We should clear slots after getting result about cluster slots, because if we got exception or timeout in

Issue #1252: Fix creating a lot of new Jedis instances on unstable cluster, fix slots clearing without filling #1253

Issue #1252: Fix creating a lot of new Jedis instances on unstable cluster, fix slots clearing without filling #1253

Conversation

Spikhalskiy commented Apr 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spikhalskiy commented Apr 6, 2016

Spikhalskiy commented Apr 6, 2016

marcosnils commented Apr 6, 2016

HeartSaVioR commented Apr 6, 2016

Spikhalskiy commented Apr 6, 2016

Spikhalskiy commented Apr 6, 2016 • edited Loading

Spikhalskiy commented Apr 7, 2016

HeartSaVioR commented Apr 7, 2016

HeartSaVioR commented Apr 7, 2016

Spikhalskiy commented Apr 7, 2016

marcosnils commented Apr 7, 2016

Spikhalskiy commented Apr 20, 2016

Choose a reason for hiding this comment

Spikhalskiy Jul 8, 2016 • edited Loading

Choose a reason for hiding this comment

xetorthio commented Jul 8, 2016

Spikhalskiy commented Jul 8, 2016 • edited Loading

xetorthio commented Jul 8, 2016

marcosnils commented Jul 19, 2016

Spikhalskiy commented Apr 6, 2016 •

edited

Loading

Spikhalskiy Jul 8, 2016 •

edited

Loading

Spikhalskiy commented Jul 8, 2016 •

edited

Loading