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
A52: gRPC xDS Custom Load Balancer Configuration #298
Conversation
This change is explained in detail in the custom LB config proposal: grpc/proposal#298
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.
This looks really good! Comments are mostly minor.
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.
This looks great!
This LB is the parent for weighted_target and will configure it based on the child policy it gets in its configuration and locality weights that come in a ResolvedAddresses attribute. Described in [A52: gRPC xDS Custom Load Balancer Configuration](grpc/proposal#298)
This change is explained in detail in the custom LB config proposal: grpc/proposal#298
A52-xds-custom-lb-policies.md
Outdated
|
||
The load balancer config portion of the CDS update struct will change to take the following form: | ||
|
||
```json |
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.
This isn't actually JSON though, right? This is for the internal struct. Seems we should use a programming language syntax.
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.
I was just going for some language neutral syntax. Switched to C.
```textproto | ||
{ // Cluster | ||
load_balancing_policy: { | ||
policies: [ |
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.
[
for messages doesn't look like textproto format. The syntax highlighter is also complaining about this.
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.
Textproto does actually support [...]
for repeated fields these days.
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.
@markdroth, it supports [...]
it for repeated messages? I've only seen it for repeated scalars. (In text_format.h, I think that is SetUseShortRepeatedPrimitives(true)
.)
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.
Seems to only be mentioned in Google internal documentation, but go/textformat-spec does include [...] support for repeated messages.
Renames the LegacyLoadBalancerConfigFactory to just LoadBalancerConfigFactory and gives it responsibility for both the legacy and the new LB config mechanism. The new configuration mechanism is exaplained in gRFC A52: grpc/proposal#298
Renames the LegacyLoadBalancerConfigFactory to just LoadBalancerConfigFactory and gives it responsibility for both the legacy and the new LB config mechanism. The new configuration mechanism is exaplained in gRFC A52: grpc/proposal#298
Renames the LegacyLoadBalancerConfigFactory to just LoadBalancerConfigFactory and gives it responsibility for both the legacy and the new LB config mechanism. The new configuration mechanism is exaplained in gRFC A52: grpc/proposal#298
Renames the LegacyLoadBalancerConfigFactory to just LoadBalancerConfigFactory and gives it responsibility for both the legacy and the new LB config mechanism. The new configuration mechanism is exaplained in gRFC A52: grpc/proposal#298
Renames the LegacyLoadBalancerConfigFactory to just LoadBalancerConfigFactory and gives it responsibility for both the legacy and the new LB config mechanism. The new configuration mechanism is explained in gRFC A52: grpc/proposal#298
A52-xds-custom-lb-policies.md
Outdated
{ | ||
... | ||
|
||
"lb_policy_config": <array of LoadBalancingConfig JSON objects> |
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.
Can you add a comment as to why this is an array? (since by the point this JSON will be created, we would have only a single LoadBalancingConfig object)
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.
It was just for compatibility with existing C config parsing libraries that expect to receive an array.
…#9141) Instead of providing round robin or least request configurations directly, ClientXdsClient now wraps them in a WRR locality config. ClusterResolverLoadBalancer passes this configuration directly to PriorityLoadBalancer to use as the endpoint LB policy it provides to ClusterImplLoadBalancer. A new ResolvedAddresses attribute is also set that has all the locality weights. This is needed by WrrLocalityLoadBalancer when it configures WeightedTargetLoadBalancer. Renames the LegacyLoadBalancerConfigFactory to just LoadBalancerConfigFactory and gives it responsibility for both the legacy and the new LB config mechanism. The new configuration mechanism is explained in gRFC A52: grpc/proposal#298
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 great, modulo one small cosmetic comment.
A new proposal for refactoring how xDS LB policy configuration is processed and how custom LB policies can be configured.