Don't call cmp in non testing file #5370
Conversation
| // EqualODOnly returns whether the LBConfig is same with the parameter outside | ||
| // of the child policy, only comparing the Outlier Detection specific | ||
| // configuration. | ||
| func (lbc *LBConfig) EqualODOnly(lbc2 *LBConfig) bool { |
There was a problem hiding this comment.
Nit:
EqualIgnoringChildPolicy?
"ODOnly" seems less informative to me since we're comparing OD LB policy structs in the OD package.
| // EqualODOnly returns whether the LBConfig is same with the parameter outside | ||
| // of the child policy, only comparing the Outlier Detection specific | ||
| // configuration. | ||
| func (lbc *LBConfig) EqualODOnly(lbc2 *LBConfig) bool { |
There was a problem hiding this comment.
Also, we should probably have a test of this function that will fail if we add new fields to the config struct.
Even a purely change-detector test would help; something like:
func TestEqualFields(t *testing.T) {
fields := map[string]bool{"LoadBalancingConfig": true, "Interval": true, "BaseEjectionTime": true, ....}
t := reflect.TypeOf((*LBConfig)(nil))
for i := 0; i < t.NumField(); i++ { // that's probably not the right method name
if n := t.Field(i).Name; !fields[n] {
t.Error("New field %q; update this test and EqualIgnoringChildPolicy", n)
}
}
}There was a problem hiding this comment.
Added this change-detector test for every node in the config tree.
| // EqualODOnly returns whether the LBConfig is same with the parameter outside | ||
| // of the child policy, only comparing the Outlier Detection specific | ||
| // configuration. | ||
| func (lbc *LBConfig) EqualODOnly(lbc2 *LBConfig) bool { |
| // EqualODOnly returns whether the LBConfig is same with the parameter outside | ||
| // of the child policy, only comparing the Outlier Detection specific | ||
| // configuration. | ||
| func (lbc *LBConfig) EqualODOnly(lbc2 *LBConfig) bool { |
There was a problem hiding this comment.
Added this change-detector test for every node in the config tree.
| } | ||
|
|
||
| func TestEqualFieldsLBConfig(t *testing.T) { | ||
| fields := map[string]bool{ |
There was a problem hiding this comment.
Seeing this for all the sub-messages and thinking about it more, I'd actually be fine with a 1-liner test that just compares reflect.TypeOf().NumField() to a const and calls it a day. For the future anyway...this is good too.
There was a problem hiding this comment.
Sg, I'll leave it as is for this PR but for the future I'll compare to const.
Fixes issue #5353.
The config's Equal method is called by CDS to check the equality of discovery mechanisms. However, due to the fact that the Child Policy will not be prepared by the CDS Balancer, but will be prepared by the Cluster Resolver balancer, getting rid of the check is a logical no-op.
RELEASE NOTES: None