Skip to content
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

Add option to forceReconnect API to guarantee fair server distribution #1190

Conversation

ajax-koval-d
Copy link

This addresses #1184

@@ -774,23 +798,24 @@ public void testForceReconnectQueueBehaviorCheck() throws Exception {

String subject = subject();
ForceReconnectQueueCheckDataPort.WRITE_CHECK = "PUB " + subject;
_testForceReconnectQueueCheck(subject, pubCount, subscribeTime, port, false, 0);
_testForceReconnectQueueCheck(subject, pubCount, subscribeTime, port, false, true, 0);
Copy link
Author

@ajax-koval-d ajax-koval-d Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not cover all possible combinations since in that case locally test was running 17 secs and almost never passed

@scottf
Copy link
Contributor

scottf commented Jul 30, 2024

@ajax-koval-d I'm not going to accept the PR for the following reasons.

  1. The NatsServerPool implementation matches the behavior in the other clients. Adding behavior that is not standard is confusing to developers who use multiple language clients.
  2. There is already a way to provide your own implementation of ServerPool
  3. The idea behind force reconnect is not to reconnect to the same server. It's used in error conditions, lame duck mode handling and when you specifically want to disconnect from the current server. Reconnecting to the same server defeats the purpose.
  4. For backward compatibility, all new api on existing interfaces must have a default implementation, otherwise it is a breaking change.

@scottf scottf closed this Jul 30, 2024
@scottf
Copy link
Contributor

scottf commented Jul 30, 2024

@ajax-koval-d I am willing to discuss changes to the ServerPool interface that allows signaling of state, giving more flexibility to behavior of ServerPool implementations. For instance shuffle would not make sense, but something that indicates a connection state about to change/changed.

@ajax-koval-d
Copy link
Author

@scottf Regarding point 3 - my impression was, that according to this adr one of the purposes of reconnect api is also rebalance, and current implementation does not always fulfill that.

My problem with implementing ServerPool is that I don't need to modify usual reconnect flow, all I want to change is force reconnect behaviour.

something that indicates a connection state about change/to change

Not sure that I follow you here. Do you mean to provide new method in ServerPool interface, like goingToForceReconnect so that there'll be ability to react on it in implementation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants