clusterresolver: merge P(p)arseConfig functions #5462
Conversation
| b := balancer.Get(Name) | ||
| if b == nil { | ||
| t.Fatalf("LB policy %q not registered", Name) | ||
| } | ||
| cfgParser, ok := b.(balancer.ConfigParser) | ||
| if !ok { | ||
| t.Fatalf("LB policy %q does not support config parsing", Name) | ||
| } |
There was a problem hiding this comment.
What do you think of this instead, parser := bb{};..... parser.ParseConfig() rather than this?
There was a problem hiding this comment.
I know we do such things at a lot of places. But I feel that even though my version is a little more verbose, it is using the exported API, rather than relying on internals. More often that not, I feel that in tests, this approach serves better in the long run.
There was a problem hiding this comment.
Fair enough. Doug left me that suggestion on my Outlier Detection PR mainly because I hadn't coded balancer implementation yet.
| @@ -168,18 +168,13 @@ type LBConfig struct { | |||
| XDSLBPolicy *internalserviceconfig.BalancerConfig `json:"xdsLbPolicy,omitempty"` | |||
| } | |||
|
|
|||
| const ( | |||
| rrName = roundrobin.Name | |||
| rhName = ringhash.Name | |||
There was a problem hiding this comment.
Personally, I don't mind the alias since it saves verbosity, but I'm indifferent.
There was a problem hiding this comment.
ringhash.Name is not very long compared to rhName but it is way more descriptive.
| @@ -168,18 +168,13 @@ type LBConfig struct { | |||
| XDSLBPolicy *internalserviceconfig.BalancerConfig `json:"xdsLbPolicy,omitempty"` | |||
| } | |||
|
|
|||
| const ( | |||
| rrName = roundrobin.Name | |||
| rhName = ringhash.Name | |||
| b := balancer.Get(Name) | ||
| if b == nil { | ||
| t.Fatalf("LB policy %q not registered", Name) | ||
| } | ||
| cfgParser, ok := b.(balancer.ConfigParser) | ||
| if !ok { | ||
| t.Fatalf("LB policy %q does not support config parsing", Name) | ||
| } |
There was a problem hiding this comment.
Fair enough. Doug left me that suggestion on my Outlier Detection PR mainly because I hadn't coded balancer implementation yet.
Fixes #5369
Apart from fixing the problem mentioned in #5369, this PR also performs the following cleanup:
roundrobin.Nameandringhash.Nameand use the actual consts themselvesring_hashis implementedRELEASE NOTES: none