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

Disable RRLB health-checking for DiscoveryStrategy.ON_NEW_CONNECTION #2772

Merged
merged 2 commits into from
Dec 7, 2023

Conversation

idelpivnitskiy
Copy link
Member

Motivation:

When DiscoveryStrategy.ON_NEW_CONNECTION mode is used, LB will have a single unresolved entry, the actual IP address will be resolved by a ConnectionFactory. We should not run health-checking logic in this case because it can lead to "all unhealthy" state. We should run a new connection attempt all the time and fail with an exception from transport.

Modifications:

  • Configure a custom loadBalancerFactory on a client builder when DiscoveryStrategy.ON_NEW_CONNECTION is used;

Result:

No health-checking for an unresolved address when
DiscoveryStrategy.ON_NEW_CONNECTION is used.

Motivation:

When `DiscoveryStrategy.ON_NEW_CONNECTION` mode is used, LB will have a
single unresolved entry, the actual IP address will be resolved by a
`ConnectionFactory`. We should not run health-checking logic in this
case because it can lead to "all unhealthy" state. We should run a new
connection attempt all the time and fail with an exception from
transport.

Modifications:

- Configure a custom `loadBalancerFactory` on a client builder when
`DiscoveryStrategy.ON_NEW_CONNECTION` is used;

Result:

No health-checking for an unresolved address when
`DiscoveryStrategy.ON_NEW_CONNECTION` is used.
@bryce-anderson
Copy link
Contributor

Code change looks good. Is this worth adding a test?

It looks like the load balancer id is not unique which is the same problem for all load balancers generated by fromDefaults(). Is that something we should address later?

@idelpivnitskiy
Copy link
Member Author

It looks like the load balancer id is not unique which is the same problem for all load balancers generated by fromDefaults(). Is that something we should address later?

The original intend was that id represents who is creating a load balancer factory. When fromDefaults() is used, it means that ST creates it with default settings. When users create their custom LB, they suppose to provide a different id so that providers will see the difference between the "default one" and a "custom one".

I'm agreed that there could be use-cases, when users need to change an LB config for a specific client, even if it was fromDefaults() originally. In this case, we need something more than just id, maybe a bundle of id and targetResource. We can keep this in mind for the new LoadBalancerBuilder you are working on. Not sure if there is currently a use-case for RoundRobinLoadBalancer.

Is this worth adding a test?

Good call, will add one.

@idelpivnitskiy idelpivnitskiy merged commit 3266749 into apple:main Dec 7, 2023
15 checks passed
@idelpivnitskiy idelpivnitskiy deleted the health-check branch December 7, 2023 17:56
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.

None yet

2 participants