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

loadbalancer-experimental: Add default policies #3135

Conversation

bryce-anderson
Copy link
Contributor

Motivation:

Users currently need to understand a lot about the different policies in order to make a good decision.

Modifications:

Add default connection pool and load balancing policies. This makes it easy for users to pick the recommended default. This also gives us as maintainers a place to change the recommended default.

Motivation:

Users currently need to understand a lot about the different policies
in order to make a good decision.

Modifications:

Add default connection pool and load balancing policies. This makes it
easy for users to pick the recommended default. This also gives us as
maintainers a place to change the recommended default.
* @param <C> the concrete type of the {@link LoadBalancedConnection}
* @return the recommended default {@link ConnectionPoolPolicy}.
*/
public static <C extends LoadBalancedConnection> ConnectionPoolPolicy<C> defaultPolicy() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the benefit of having a default policy vs setting linearSearch as the default in the builder?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think my concern is that default is relative and doesn't say what it does. If the intent is that it can change then the behavior is potentially not consistent even if the default works for a user in one release but is not the appropriate one in a subsequent release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conceptually, it crystalizes the notion of a recommended default for both ourselves and users.

Practically, it gives us a central place to migrate what the default is: if users have set the value to the default they already have yielded to the best judgement of the library. Admittedly, it's unlikely that users are going to change a policy to the default but it may or may not be helpful for intermediate libraries.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not fully for/against this change but at least wanted to probe a little to see if it's completely necessary. Do we do this anywhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the intent is that it can change then the behavior is potentially not consistent even if the default works for a user in one release but is not the appropriate one in a subsequent release.

I think that is the point: it gives us a central leverage point to change behavior and the API is more explicit that users are yielding to our judgement. Admittedly changing behavior may cause regressions, but unless we want to cement the default behavior forever that is going to be the case whether we define the default in a public API as in here or if we do it as an internal detail of the DefaultLoadBalancerBuilder.

We don't use it often, but it can be found in GrpcExecutionStrategies and FlushStrategies.

I'm not adamant about this change and it doesn't have to happen now.

Copy link
Member

Choose a reason for hiding this comment

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

FlushStrategies is internal API. GrpcExecutionStrategies/HttpExecutionStrategies have different meaning, it's not a placeholder for a default value, it's actually the "default behavior" that is based on what we can compute at the build time. Naming is hard, but the point is that it's different from "return offloadAll()today andreturn offloadNone()` if we changed our mind. So, I would say we don't currently have a public API with the similar goal like in this PR.

Should we take it or not? I don't have a strong opinion leaning to any side. In the past, when we frequently had version increments (..., 0.40.0, 0.41.0, 0.42.0) we said that we intentionally do not clarify default values, not in javadoc nor in public API until we release 1.0.0. That gives us some flexibility to adjust. Right now, we stuck with 0.42.x until forever I guess.

I will try to ask a different question: what will be benefit for users to touch public defaultPolicy() method instead of not touching that setter on LB builder at all? Seems like if they wanna change DEFAULT_LINEAR_SEARCH_SPACE or other params on RoundRobinLoadBalancingPolicyBuilder, they will move away from defaultPolicy(). Then if we change it later, they won't have a difference bcz it's overridden with their settings. If that will only work for users who don't touch builder at all, then we can just have it as private API instead of making it public.

@mgodave mgodave self-requested a review December 11, 2024 18:19
@bryce-anderson
Copy link
Contributor Author

For the moment it seems the momentum is against merging this right now. Since it's easy to add it later but not easy to take it back, I'm going to close it for the time being and we can re-evaluate it later if we want to.

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.

3 participants