upstream: allow a cluster to provide its own load balancer

[mattklein123]

Certain clusters have cluster specific load balancers. This change
allows a cluster to explicitly provide one, both allowing extension
clusters to easily provide a dedicated load balancer, as well as
allowing for future cleanup of the original DST LB configuration.

This change is needed for Redis Cluster as well as
#1606.

Risk Level: Low
Testing: New UTs and integration tests.
Docs Changes: N/A
Release Notes: N/A

[mattklein123]

@HenryYYang this should be what you need for Redis Cluster. I left you a TODO to implement. I will also build on this for #1606

[snowp]

Looks good to me, just one minor comment

[HenryYYang]

I ended up implementing something similar, except I didn't change the ClusterFactory, I put the loadBalancerFactory() method on the cluster object itself. #6895 . Sorry for the miscommunication.

[mattklein123]

I ended up implementing something similar, except I didn't change the ClusterFactory, I put the loadBalancerFactory() method on the cluster object itself. #6895 . Sorry for the miscommunication.

@HenryYYang I considered doing it this way, but IMO it felt cleaner to have the factory return a single LB at cluster construction time since there should be a 1:1 correspondence between them, vs. an API hanging off a cluster that could be called multiple times. It's a minor point but it felt a bit cleaner to me. @snowp @HenryYYang WDYT?

[HenryYYang]

I ended up implementing something similar, except I didn't change the ClusterFactory, I put the loadBalancerFactory() method on the cluster object itself. #6895 . Sorry for the miscommunication.

@HenryYYang I considered doing it this way, but IMO it felt cleaner to have the factory return a single LB at cluster construction time since there should be a 1:1 correspondence between them, vs. an API hanging off a cluster that could be called multiple times. It's a minor point but it felt a bit cleaner to me. @snowp @HenryYYang WDYT?

Yes, both way works. I'll change my code to build on top of this. Thanks 👍

[htuch]

@mattklein123 from an API perspective, I'm wondering how this interacts with general purpose LB extensibility (#5598). Let's say we want to be able to support:

1. Custom cluster types
2. Custom cluster types with custom LB.
3. Baked in cluster types with custom LB.
   In the case of (3), we might have opaque configuration to feed in to the LB extension. I think with the API here, we might be able to support (3) by adding in some opaque LB config still later. Is this how you see it playing out?

[mattklein123]

@htuch yes exactly. I think we can easily add custom LBs in a similar way that @HenryYYang added custom cluster support. I think in the end we will need all 3 extension points. This change is nice in that it removes a config hack in which a specific LB and cluster need to be configured together for things to work correctly.

[snowp]

LGTM. +1 to this approach over adding it as a function on the cluster

[htuch]

@mattklein123 can we rename to CustomLb or something then, since it won't always be "cluster provided"? LGTM otherwise.

[mattklein123]

@mattklein123 can we rename to CustomLb or something then, since it won't always be "cluster provided"? LGTM otherwise.

IMO these are different cases. In this case, we are saying that the cluster must provide the LB. I think this is different from a custom LB that applies to any cluster? I think in the latter case we would have different config? WDYT?

[htuch]

@mattklein123 ack, that's reasonable, we can have two enum values then. Ship it.