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

Add a method for resolver to report error #2102

Closed
menghanl opened this issue May 23, 2018 · 13 comments · Fixed by #2951
Closed

Add a method for resolver to report error #2102

menghanl opened this issue May 23, 2018 · 13 comments · Fixed by #2951

Comments

@menghanl
Copy link
Contributor

If a resolver could not resolve a name, there's currently no way to report error back to ClientConn, and RPCs on the ClientConn will fail with timeout.

This method allows resolver to report error to ClientConn, and failfast RPCs should fail with this error (or RPC errors that contain info from this error).

@peats-bond
Copy link

Can we get an update on the status/timeline of this? In #2785, @menghanl you said that this was being actively worked on.

@menghanl
Copy link
Contributor Author

The plan was to do this after #2732, because they both add features to resolver/balancer. But #2732 got delayed by discussion on some details.
This is not necessary blocked by that PR, I think we can do this first. I will try to put something together this week.

@peats-bond
Copy link

Perfect, thanks for the update!

@peats-bond
Copy link

@menghanl any update?

@menghanl
Copy link
Contributor Author

menghanl commented Jun 3, 2019

The changes are in #2831. It might need more discussion.
Please take a look and comment. Thanks!

@menghanl
Copy link
Contributor Author

menghanl commented Jun 6, 2019

This error handling logic in resolver gets complicated with service config. (e.g. What to do if we get an error fetching addresses, but get service config? Do we apply the service config or also drop it? What if we get addresses but failed for service config?). We may end up needing two errors.

This is still under discussion, we will send an update when there's a conclusion.

@peats-bond
Copy link

Gentle ping :)

@menghanl
Copy link
Contributor Author

@dfawley is working on this now. The PR will support this and also errors for service config. The PR should be ready soon (within two weeks).

@jeffbean
Copy link

any updates would be great :shipit:. trying to estimate if this how much risk this is to our project.

@dfawley
Copy link
Member

dfawley commented Aug 28, 2019

This is PR #2951.

@jeffbean
Copy link

This is PR #2951.

🤦‍♂ ty! I need more coffee.

@stale
Copy link

stale bot commented Sep 6, 2019

This issue is labeled as requiring an update from the reporter, and no update has been received after 7 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@stale stale bot added the stale label Sep 6, 2019
@dfawley
Copy link
Member

dfawley commented Sep 6, 2019

Something seems to be broken. I'm going to disable the stale bot for now.

@stale stale bot removed the stale label Sep 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging a pull request may close this issue.

4 participants