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

Move name resolution retry from managed channel to name resolver (take #2) #9812

Merged
merged 12 commits into from
Feb 4, 2023

Conversation

temawi
Copy link
Contributor

@temawi temawi commented Jan 12, 2023

This change has these main aspects to it:

  1. Removal of any name resolution responsibility from ManagedChannelImpl
  2. Creation of a new RetryScheduler to own generic retry logic
    • Can also be used outside the name resolution context
  3. Creation of a new RetryingNameScheduler that can be used to wrap any polling name resolver to add retry capability
  4. Use of a temporary callback object in ResolutionResult attributes to allow ManagedChannelImpl to indicate if resolved addresses were accepted. This can be removed once the Listener2.onResult() API can be updated to return a boolean for this purpose.

This change has these main aspects to it:

1. Removal of any name resolution responsibility from ManagedChannelImpl
2. Creation of a new RetryScheduler to own generic retry logic
     - Can also be used outside the name resolution context
3. Creation of a new RetryingNameScheduler that can be used to wrap any
   polling name resolver to add retry capability
4. A new facility in NameResolver to allow implementations to notify
   listeners on the success of name resolution attempts
     - RetryingNameScheduler relies on this
- Do not change the public API
- Use a hacky internal callback
- More unit test coverage
@temawi
Copy link
Contributor Author

temawi commented Jan 12, 2023

cc: @larry-safran

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

There's two problems not addressed:

  1. We need a utility for third-party polling NameResolvers to use
  2. We need to give time for migration. This could be handled by using RetryingNameResolver in ManagedChannelImpl

If we want to implement this over multiple PRs, then ManagedChannelImpl could be the only user of RetryingNameResolver for now and we add the API in a later PR.

Copy link
Contributor Author

@temawi temawi left a comment

Choose a reason for hiding this comment

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

I made some changes based on a few assumptions. PTAL and let me know if it doesn't look right.

@temawi temawi requested a review from ejona86 February 4, 2023 01:10
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

"Easy"

@temawi temawi merged commit fcb5c54 into grpc:master Feb 4, 2023
@temawi temawi deleted the dns-backoff branch February 4, 2023 02:23
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants