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: do not apply default service config upon receipt of invalid service config #5257

Merged

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Mar 21, 2022

In #5238, we made a change to apply the default service config when grpc receives invalid service config from the name resolver (and a default service config exists).

But this does not sit well with 4.2:

If the client has not previously selected a valid service config or no service config,
for example a new client, then it treats the resolution attempt as having failed
(e.g., for a polling-based resolver, it should retry the query after appropriate backoff).
In other words, the channel will remain in state TRANSIENT_FAILURE until the
resolver returns a valid response.

This PR does the following:

  • undoes some of the changes from grpc: handle invalid service configs by applying the default, if applicable #5238, by not applying the default service config
  • Renames test/resolver_test.go to test/config_selector_test.go (since it contained only config selector related tests)
  • Adds a new test/resolver_test.go which contains tests for different error handling scenarios upon receipt of resolver update. This pulls in a couple of tests from resolver_conn_wrapper_test.go, which is now deleted.

RELEASE NOTES: none

@easwars easwars requested a review from dfawley March 21, 2022 19:49
@easwars easwars added this to the 1.46 Release milestone Mar 21, 2022
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Nice, thanks for adding the new test cases. Did you check in / to see if similar tests already existed there? I would not be surprised if there were similar ones, but also not surprised if there were not. :)

@@ -0,0 +1,214 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this with the proper git magic to make it show up that way, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Earlier, I think I had done a git mv test/resolver_test.go test/config_selector_test.go. But then, added the new tests to a new file test/resolver_test.go. But git knew it as a old file. Hence the confusion. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I had to put the tests from resolver_conn_wrapper_test.go and the new tests into resolver_update_test.go.

@dfawley dfawley assigned easwars and unassigned dfawley Mar 21, 2022
@easwars easwars force-pushed the dont_apply_default_sc_upon_resolver_error branch from 7d2ab10 to d6329e5 Compare March 22, 2022 00:12
@easwars
Copy link
Contributor Author

easwars commented Mar 22, 2022

Nice, thanks for adding the new test cases. Did you check in / to see if similar tests already existed there? I would not be surprised if there were similar ones, but also not surprised if there were not. :)

I had done some basic grepping, and the only tests were the ones in the now deleted resolver_conn_wrapper_test.go. They were incomplete.

@easwars easwars merged commit 597e5d1 into grpc:master Mar 22, 2022
@easwars easwars deleted the dont_apply_default_sc_upon_resolver_error branch March 22, 2022 15:51
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants