priority: add policy config parsing #3936
Conversation
There was a problem hiding this comment.
These changes look good to me overall. But should this policy live under xds/internal.
The LB policy refactoring doc says the following:
Note that this policy is not xds-specific. It does not interact with the XdsClient and
contains no xds-specific code. It is intended to be a generic policy that could be used in
any non-xds context.
| prioritiesSet := make(map[string]bool) | ||
| for _, name := range cfg.Priorities { | ||
| if _, ok := cfg.Children[name]; !ok { | ||
| return nil, fmt.Errorf("child name %v is not found in children %+v", name, cfg.Children) |
There was a problem hiding this comment.
Maybe the error message could be something like:
fmt.Errorf("LB policy name %q found in Priorities field (%v) is not found in Children field (%+v)", name, cfg.Priorities, cfg.Children)
| } | ||
| for name := range cfg.Children { | ||
| if _, ok := prioritiesSet[name]; !ok { | ||
| return nil, fmt.Errorf("child with name %v is not used in priorities %+v", name, cfg.Children) |
There was a problem hiding this comment.
Similar comment about this error string.
There was a problem hiding this comment.
Thanks for the review.
This is in xds/internal for the same reason that weighted_target is in xds/internal.
We will use these ourselves, for the xds policies first. When they are proved to work (and also we will be confident about their behavior), we will them out to an exported package.
| prioritiesSet := make(map[string]bool) | ||
| for _, name := range cfg.Priorities { | ||
| if _, ok := cfg.Children[name]; !ok { | ||
| return nil, fmt.Errorf("child name %v is not found in children %+v", name, cfg.Children) |
| } | ||
| for name := range cfg.Children { | ||
| if _, ok := prioritiesSet[name]; !ok { | ||
| return nil, fmt.Errorf("child with name %v is not used in priorities %+v", name, cfg.Children) |
3f2c9ed
to
273333c
Compare
Can we then make that obvious in |
| type lbConfig struct { | ||
| serviceconfig.LoadBalancingConfig | ||
|
|
||
| Children map[string]*child |
There was a problem hiding this comment.
Also a comment here about the structure of the config would be useful so that one doesn't have to look at the design doc to understand that.
No description provided.