Fix retries logic #2351
Fix retries logic #2351
Conversation
Some of these fail, to be fixed up in a separate commit.
| } | ||
|
|
||
| return runWithRetries(slot, attempts - 1, tryRandomNode, redirect); | ||
| return runWithRetries(slot, attempts - 1, true, null); |
There was a problem hiding this comment.
This would heavily affect current applications. Connection exceptions can happen for silliest of the reasons. So we can't backoff right after a connection exception. There should a balance between retry and backoff. For example, a slot based node or a redirected node should be given at least 2/3 consecutive tries before backing off.
In all of the cases, maxAttempts, remaining attempts, renewSlotCache() should be kept in mind.
There was a problem hiding this comment.
I'll just try to rephrase what you said so I know I understand:
- With this change, we'd start trying random nodes on the first connection exception
- In your opinion, what we should do is:
- First, try the exact same operation a few times in a row
- If that doesn't help, start trying random nodes
I think this sounds reasonable, just want to ensure I change the right thing!
There was a problem hiding this comment.
@walles Yes, you've got it right.
|
Let's just revisit this after #2355 has gone in. |
2 participants
Done as part of researching #2347.
This change adds tests for
JedisClusterCommand.runWithRetries(), and fixes two logic errors in said method.It contains no backoff, we thought it best to do this piecemeal and do that in another PR.