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

grpc: add a field to represent receipt of a good resolver update #5235

Closed
wants to merge 1 commit into from

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Mar 10, 2022

Add a field in ClientConn to represent the receipt of a good resolver update.

  • This field will be guarded by cc.mu, and will be used to determine if a valid balancer is expected to be configured on the channel.
  • Currently this is done by cc.balancerWrapper != nil. We plan to change the ccBalancerWrapper implementation to wrap a gracefulswitch.Balancer instead of a regular balancer.Balancer and once we do that, the cc.balancerWrapper field will be initialized at dial time and will never become nil.

This PR also makes the following changes to the implementation (without changing behavior):

  • The existing implementation uses the following flow:
    • cc.updateResolverState calls cc.applyServiceConfigAndBalancer when a valid service config is received and calls cc.maybeApplyDefaultServiceConfig otherwise
    • cc.maybeApplyDefaultServiceConfig determines the default service config to apply and calls cc.applyServiceConfigAndBalancer
    • cc.applyServiceConfigAndBalancer applies the service config and if the LB policy changes, it calls cc.switchBalancer
    • There is little documentation around what is happening and why.

The PR introduces the following changes:

  • cc.updateResolverState calls cc.onResolverError if the update contains an error
  • If there is no error in the update, it calls cc.tryApplyServiceConfig which attempts to apply the most appropriate service config, and returns an error if the service configs are invalid
    • cc.tryApplyServiceConfig is written to align more with the pseudo-code we use to reason about expected behavior
    • cc.tryApplyServiceConfig might call cc.maybeApplyDefaultServiceConfig or cc.applyServiceConfigAndBalancer
  • cc.applyServiceConfigAndBalancer is split up into cc.applyServiceConfig and cc.applyBalancer
  • All of these functions are heavily documented now

RELEASE NOTES: none

@easwars easwars requested a review from dfawley March 10, 2022 17:15
@easwars easwars requested a review from zasweq March 10, 2022 17:15
@easwars easwars added the Type: Internal Cleanup Refactors, etc label Mar 10, 2022
@easwars easwars added this to the 1.46 Release milestone Mar 10, 2022
@easwars
Copy link
Contributor Author

easwars commented Mar 11, 2022

Closing this PR in favor of #5238, and will follow-up with a refactoring/documenting PR.

@easwars easwars closed this Mar 11, 2022
@easwars easwars deleted the good_resolve_event branch March 30, 2022 14:47
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2022
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.

None yet

2 participants