-
Notifications
You must be signed in to change notification settings - Fork 150
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
reconnect-jitter-handler-serverinfo-tests #391
reconnect-jitter-handler-serverinfo-tests #391
Conversation
byte[] getNonce(); | ||
boolean isJetStreamAvailable(); | ||
int getClientId(); | ||
String getClientIp(); |
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.
Decided on the string...
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.
Agreed. The InetAddress has static methods that the dev can call directly to find out info you were describing.
// prioritize discovered over configured | ||
List<String> servers = new ArrayList<>(); | ||
addDiscoveredServers(servers); | ||
shuffleWithoutCurrent(servers); |
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.
Couldn't shuffle be called once after adding discovered and then configured?
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.
The thought here was that discovered servers are the most viable and contain the list of configured servers, although possible in a ip form instead of a dns name form. In fact I'm not even sure of the value of adding the configured servers in the list once the discovered are populated.
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.
Discovered servers should be only servers that weren't originally configured. For example, if I have servers A, B, and C configured in the client then added D and E, the client would have A,B,C as configured servers and D and E as a discovered servers. Randomization should be applied to the whole list (sans current during reconnect), so each server will be weighted equally in probability w/r/t reconnect order. Someday we'll have classes of servers for regional preference (e.g. try this set first, then if all those fail, try another set), but that's TBD at the moment.
There is a chance for duplicates based on IP vs hostname, but server configuration allows ops to handle that.
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.
Looks good - just a few questions...
…calhost cluster scenario
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 - Thanks!
No description provided.