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

config: remove retry disable via environment variable #4922

Merged
merged 1 commit into from
Nov 9, 2021

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Nov 2, 2021

RELEASE NOTES:

  • config: remove retry disable via environment variable

@dfawley dfawley added the Type: Behavior Change Behavior changes not categorized as bugs label Nov 2, 2021
@dfawley dfawley added this to the 1.43 release milestone Nov 2, 2021
@dfawley dfawley requested a review from zasweq November 2, 2021 16:40
Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

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

This looks fine, but I think we should wait for #4925 to get merged and rebase this onto that master. #4925 changes behavior server side because route specifier is now required, which might (but I don't think so) break one of these tests which used to be gated by an env var.

@menghanl
Copy link
Contributor

menghanl commented Nov 3, 2021

Should we just keep RBAC env var? We have other env vars older than RBAC. And RBAC adds a new requirement of the responses, which is more likely to break if the control plane isn't up-to-date (#4924)

We want to remove retry because of the confusion caused by two retry vars.

@menghanl menghanl assigned dfawley and unassigned zasweq Nov 3, 2021
@dfawley
Copy link
Member Author

dfawley commented Nov 3, 2021

We have other env vars older than RBAC

The ones which are tested sufficiently should be removed, too. I hadn't been looking at the others, but it seems like client side security and possibly ring hash should also be removed?

We want to remove retry because of the confusion caused by two retry vars.

We want to remove retry because we believe it is sufficiently tested. Having too many flags is not a good idea. They are intended to be short-term testing tools, primarily.

@dfawley dfawley changed the title config: remove retry and RBAC disable via environment variables config: remove retry disable via environment variables Nov 9, 2021
@dfawley dfawley changed the title config: remove retry disable via environment variables config: remove retry disable via environment variable Nov 9, 2021
@dfawley dfawley requested a review from menghanl November 9, 2021 19:24
@dfawley dfawley assigned menghanl and unassigned dfawley Nov 9, 2021
@dfawley
Copy link
Member Author

dfawley commented Nov 9, 2021

Removed the removal of RBAC control. This just deletes the retry control now.

@menghanl menghanl assigned dfawley and unassigned menghanl Nov 9, 2021
@dfawley dfawley merged commit c25a52b into grpc:master Nov 9, 2021
@dfawley dfawley deleted the always_on branch November 9, 2021 21:06
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Behavior Change Behavior changes not categorized as bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants