-
Notifications
You must be signed in to change notification settings - Fork 151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Randomize implicit servers #376
Conversation
Signed-off-by: Colin Sullivan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a note about the copyright date for the new test file.
@@ -0,0 +1,139 @@ | |||
// Copyright 2015-2020 The NATS Authors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is new file, start at 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix... Thanks!
Signed-off-by: Colin Sullivan <[email protected]>
{ | ||
// pick a random spot to add the server. | ||
var randElem = sList.ElementAt(rand.Next(sList.Count)); | ||
sList.AddAfter(sList.Find(randElem), s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on latest comment on the Go client, I wonder if this need to be updated. Basically in Go I was shuffling the pool after adding all new URLs (when processing INFO). But the point made was that possibly the first URL in the pool would be moved (say to position 2 (index 1)), and on reconnect that would be the very next one to be tried although we just disconnected from it. Let me know if I make sense (note the Go PR has not been updated as I type this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per discussions in Slack, we've concluded that this achieves the same result, so I'll keep this as is. Appreciate the catch here!
See nats-io/nats.go#565.
The server list is randomized on initial setup, then implicit servers are added into the list in a random position. The NoRandomize option is honored, which will keep the initial server list static and place implicit servers at the end of the server list.
This PR also includes unit testing for the server pool.
Signed-off-by: Colin Sullivan [email protected]