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

xds: populate envoy RetryPolicy with no retryOn to resolver #8511

Merged
merged 1 commit into from
Sep 13, 2021

Conversation

dapengzhang0
Copy link
Member

@dapengzhang0 dapengzhang0 commented Sep 10, 2021

Envoy RetryPolicy with empty retryOn should not be ignored as no retry config when selecting Route config. Therefore, if xDS update for a route contains a RetryPolicy that has no RetryOn value that we support, but the virtual host config does, xds client should choose the Envoy RetryPolicy from the route (even with no RetryOn), rather than choosing the one from virtual host, and try to convert it into grpc RetryPolicy, and end up with no retry.

@dapengzhang0 dapengzhang0 added the TODO:backport PR needs to be backported. Removed after backport complete label Sep 10, 2021
@ejona86
Copy link
Member

ejona86 commented Sep 13, 2021

It would be helpful to describe what behavior change happens with this PR. At a glance it looks like it should have the same behavior as previously. Calling out that it impacts which config is used would be helpful.

@dapengzhang0 dapengzhang0 merged commit 9ff5405 into grpc:master Sep 13, 2021
@dapengzhang0 dapengzhang0 deleted the allow-empty-retryable-code branch September 13, 2021 15:31
dapengzhang0 added a commit to dapengzhang0/grpc-java that referenced this pull request Sep 13, 2021
Envoy RetryPolicy with empty retryOn should not be ignored as no retry config when selecting Route config. Therefore, if xDS update for a route contains a RetryPolicy that has no RetryOn value that we support, but the virtual host config does, xds client should choose the Envoy RetryPolicy from the route (even with no RetryOn), rather than choosing the one from virtual host, and try to convert it into grpc RetryPolicy, and end up with no retry.
dapengzhang0 added a commit to dapengzhang0/grpc-java that referenced this pull request Sep 13, 2021
Envoy RetryPolicy with empty retryOn should not be ignored as no retry config when selecting Route config. Therefore, if xDS update for a route contains a RetryPolicy that has no RetryOn value that we support, but the virtual host config does, xds client should choose the Envoy RetryPolicy from the route (even with no RetryOn), rather than choosing the one from virtual host, and try to convert it into grpc RetryPolicy, and end up with no retry.
dapengzhang0 added a commit that referenced this pull request Sep 13, 2021
Envoy RetryPolicy with empty retryOn should not be ignored as no retry config when selecting Route config. Therefore, if xDS update for a route contains a RetryPolicy that has no RetryOn value that we support, but the virtual host config does, xds client should choose the Envoy RetryPolicy from the route (even with no RetryOn), rather than choosing the one from virtual host, and try to convert it into grpc RetryPolicy, and end up with no retry.
dapengzhang0 added a commit that referenced this pull request Sep 13, 2021
Envoy RetryPolicy with empty retryOn should not be ignored as no retry config when selecting Route config. Therefore, if xDS update for a route contains a RetryPolicy that has no RetryOn value that we support, but the virtual host config does, xds client should choose the Envoy RetryPolicy from the route (even with no RetryOn), rather than choosing the one from virtual host, and try to convert it into grpc RetryPolicy, and end up with no retry.
@dapengzhang0 dapengzhang0 removed the TODO:backport PR needs to be backported. Removed after backport complete label Sep 13, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 13, 2021
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