Skip to content
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

Don't call cmp in non testing file #5370

Merged
merged 2 commits into from
May 23, 2022

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented May 19, 2022

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

// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

EqualIgnoringChildPolicy?

"ODOnly" seems less informative to me since we're comparing OD LB policy structs in the OD package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched.

// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
		}
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this change-detector test for every node in the config tree.

Copy link
Contributor Author

@zasweq zasweq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comments :D!

// 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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched.

// 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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this change-detector test for every node in the config tree.

}

func TestEqualFieldsLBConfig(t *testing.T) {
fields := map[string]bool{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sg, I'll leave it as is for this PR but for the future I'll compare to const.

@zasweq zasweq merged commit ed75225 into grpc:master May 23, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants