[FIX] Reconnect wait was not being honored #330
Conversation
Signed-off-by: Colin Sullivan <colin@synadia.com>
Signed-off-by: Colin Sullivan <colin@synadia.com>
Signed-off-by: Colin Sullivan <colin@synadia.com>
There was a problem hiding this comment.
Should we try to fix this one in the same change? #290
Basically, all other clients behave different and ensure that they wait for reconnect delay before picking the same server again.
| if (isDisconnectingOrClosed() || this.isClosing()) { | ||
| break; | ||
| } | ||
|
|
||
| if (server.equals(lastServer)) { | ||
| shouldWait = true; |
There was a problem hiding this comment.
if we manage to connect, should shouldWait be set to false?
line 305
There was a problem hiding this comment.
i think so, otherwise it think we wait if we happen to connect to the last server
There was a problem hiding this comment.
Yep, good catch, thanks. I'll fix it, and move the delay up per other comments.
| @@ -325,6 +326,11 @@ void reconnect() throws InterruptedException { | |||
| if (maxTries > 0 && tries >= maxTries) { | |||
| break; | |||
| } | |||
|
|
|||
| if (shouldWait) { | |||
There was a problem hiding this comment.
shouldn't this happen inside the for loop? line 318 when we did not succeed or exceed the limits?
There was a problem hiding this comment.
it is in the while loop, but i could see it going either way for sure
There was a problem hiding this comment.
might be cleaner to move it up
| @@ -325,6 +326,11 @@ void reconnect() throws InterruptedException { | |||
| if (maxTries > 0 && tries >= maxTries) { | |||
| break; | |||
| } | |||
|
|
|||
| if (shouldWait) { | |||
There was a problem hiding this comment.
it is in the while loop, but i could see it going either way for sure
| @@ -325,6 +326,11 @@ void reconnect() throws InterruptedException { | |||
| if (maxTries > 0 && tries >= maxTries) { | |||
| break; | |||
| } | |||
|
|
|||
| if (shouldWait) { | |||
There was a problem hiding this comment.
might be cleaner to move it up
| if (isDisconnectingOrClosed() || this.isClosing()) { | ||
| break; | ||
| } | ||
|
|
||
| if (server.equals(lastServer)) { | ||
| shouldWait = true; |
There was a problem hiding this comment.
i think so, otherwise it think we wait if we happen to connect to the last server
Signed-off-by: Colin Sullivan <colin@synadia.com>
|
Thanks for the reviews. I moved the check into the loop. I also moved the test to |
|
I have no clue why but can't trigger a re-run for the single flapping CI test. Safe to merge IMO. |
There was a problem hiding this comment.
This also closes #290 as we'll sleep once we went through the entire list once.
The reconnect wait option was not being honored. Adjust the reconnect logic to honor it.
Partially resolves #329.
Resolves #290.