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.

Sign up for GitHub

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

Jump to bottom

client: return helpful error message when wait-for-ready RPCs fail with timeout #2777

Merged
merged 2 commits into from
Apr 29, 2019
Merged

client: return helpful error message when wait-for-ready RPCs fail with timeout #2777

merged 2 commits into from
Apr 29, 2019

Conversation

yuqitao
Copy link
Contributor

@yuqitao yuqitao commented Apr 16, 2019

No description provided.

@yuqitao
Copy link
Contributor Author

yuqitao commented Apr 16, 2019

#2776

@thelinuxfoundation
Copy link

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

Regards,
CLA GitHub bot

@yuqitao
Copy link
Contributor Author

yuqitao commented Apr 16, 2019

@xiang90 PTAL

@yuqitao
Copy link
Contributor Author

yuqitao commented Apr 22, 2019

("I signed it")

@yuqitao
Copy link
Contributor Author

yuqitao commented Apr 22, 2019

@menghanl PTAL

yuqitao
yuqitao commented Apr 22, 2019
View reviewed changes
picker_wrapper.go Outdated Show resolved Hide resolved
menghanl
menghanl suggested changes Apr 22, 2019
View reviewed changes
picker_wrapper.go Outdated Show resolved Hide resolved
@yuqitao yuqitao force-pushed the return-helpful-error-message branch 2 times, most recently from 6727225 to 816aae4 Compare April 24, 2019 03:57
yuqitao
yuqitao commented Apr 24, 2019
View reviewed changes
picker_wrapper.go Outdated Show resolved Hide resolved
@yuqitao yuqitao force-pushed the return-helpful-error-message branch from 816aae4 to 27a8866 Compare April 25, 2019 07:36
menghanl
menghanl reviewed Apr 26, 2019
View reviewed changes
Copy link
Contributor

@menghanl menghanl 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 fixes. Overall LGTM. One small change.

picker_wrapper.go Outdated Show resolved Hide resolved
@yuqitao yuqitao force-pushed the return-helpful-error-message branch from 27a8866 to 32875ab Compare April 26, 2019 01:36
menghanl
menghanl reviewed Apr 26, 2019
View reviewed changes
Copy link
Contributor

@menghanl menghanl left a comment

Choose a reason for hiding this comment

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

Can you also add a test for it? It should be very similar with TestFailFastRPCErrorOnBadCertificates

You can make a copy of TestFailFastRPCErrorOnBadCertificates, (and call the new one TestWaitForReadyRPCErrorOnBadCertificates maybe).
In the test, change the RPC tc.EmptyCall to an wait-for-ready RPC with a short timeout, and make sure the returned error contains the expected string.

picker_wrapper.go Outdated Show resolved Hide resolved
@yuqitao yuqitao force-pushed the return-helpful-error-message branch from 32875ab to c87c1f4 Compare April 28, 2019 07:01
@yuqitao
Copy link
Contributor Author

yuqitao commented Apr 28, 2019

Can you also add a test for it? It should be very similar with TestFailFastRPCErrorOnBadCertificates

You can make a copy of TestFailFastRPCErrorOnBadCertificates, (and call the new one TestWaitForReadyRPCErrorOnBadCertificates maybe).
In the test, change the RPC tc.EmptyCall to an wait-for-ready RPC with a short timeout, and make sure the returned error contains the expected string.
Done!

menghanl
menghanl approved these changes Apr 29, 2019
View reviewed changes
Copy link
Contributor

@menghanl menghanl left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix!

@menghanl menghanl changed the title FailFast(false), return helpful error message when context is time out client: return helpful error message when wait-for-ready RPCs fail with timeout Apr 29, 2019
@menghanl menghanl merged commit 47e1ebe into grpc:master Apr 29, 2019
@dfawley dfawley added the Type: Feature New features or improvements in behavior label May 2, 2019
@dfawley dfawley added this to the 1.21 Release milestone May 2, 2019
@jingyih
Copy link

jingyih commented May 6, 2019 •
edited
Loading

@menghanl Will this be included in the next release? Is there a scheduled release date? Thanks!

@menghanl
Copy link
Contributor

menghanl commented May 6, 2019

Yes, the next release is scheduled at May 21.

This was referenced Jul 3, 2019
Closed
Closed
@lock lock bot locked as resolved and limited conversation to collaborators Nov 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@menghanl menghanl menghanl approved these changes

Assignees

@menghanl menghanl

Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Milestone
1.21 Release
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@yuqitao @thelinuxfoundation @jingyih @menghanl @dfawley