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

xds/clusterresolver: stop forwarding UpdateSubConnState calls #6526

Merged
merged 2 commits into from
Aug 9, 2023

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Aug 9, 2023

All children use StateListener so this is unnecessary now. Also, delete associated test.

RELEASE NOTES: none

@dfawley dfawley added the Type: Internal Cleanup Refactors, etc label Aug 9, 2023
@dfawley dfawley added this to the 1.58 Release milestone Aug 9, 2023
@dfawley dfawley requested a review from zasweq August 9, 2023 20:13
Copy link
Contributor

@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.

LGTM.

Comment on lines 405 to 406
// No need to override opts.StateListener; just forward all calls to the
// child that created the SubConn.
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional nit: I don't think you need this comment anymore. If we have this comment we'd have to restate this in every balancer since we switched over.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this should have just been deleted entirely since ccWrapper embeds ClientConn. Done.

@dfawley dfawley merged commit 694cb64 into grpc:master Aug 9, 2023
1 check passed
@dfawley dfawley deleted the uscs_clusterresolver branch August 9, 2023 21:18
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants