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

[Rls] de-experimentalize RLS in XDS #33290

Merged
merged 5 commits into from Jun 2, 2023
Merged

[Rls] de-experimentalize RLS in XDS #33290

merged 5 commits into from Jun 2, 2023

Conversation

apolcyn
Copy link
Contributor

@apolcyn apolcyn commented May 31, 2023

Integration tests are passing, so we should be ready to de-experimentalize.

Related: internal bug b/265209578

@apolcyn apolcyn changed the title [XDS RLS] de-experimentalize RLS in XDS [RLS] de-experimentalize RLS in XDS May 31, 2023
@apolcyn apolcyn changed the title [RLS] de-experimentalize RLS in XDS [Rls] de-experimentalize RLS in XDS Jun 1, 2023
@apolcyn apolcyn marked this pull request as ready for review June 1, 2023 21:21
@apolcyn apolcyn requested a review from markdroth as a code owner June 1, 2023 21:21
@apolcyn apolcyn added the release notes: yes Indicates if PR needs to be in release notes label Jun 1, 2023
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

In C-core, we normally just remove the env var guard once interop tests pass rather than just flipping the default, so that we don't have to remember to come back later and remove the env var guard completely.

If you want to keep the ability to disable this feature at run-time for a short period of time just to be safe, I'm okay with that, as long as you commit to coming back and removing the env var guard after that -- let's say no longer than 1-2 releases. I don't want this cruft sitting in our code forever.

@@ -80,10 +80,9 @@

namespace grpc_core {

// TODO(donnadionne): Remove once RLS is no longer experimental
Copy link
Member

Choose a reason for hiding this comment

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

This TODO needs to stay, although you can change the assignee. We still want to get rid of this env var guard before too long.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SG, I suspect this env var will be useful at least for upcoming weeks while we iron out possible RLS issues beyond the client.

Updated the comment to remove by 1.58

@apolcyn
Copy link
Contributor Author

apolcyn commented Jun 2, 2023

bazel c/c++ tool failure

@apolcyn apolcyn merged commit 889412c into grpc:master Jun 2, 2023
63 of 65 checks passed
apolcyn added a commit to apolcyn/grpc that referenced this pull request Jun 2, 2023
Integration tests are passing, so we should be ready to
de-experimentalize.

Related: internal bug b/265209578
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Jun 2, 2023
apolcyn added a commit that referenced this pull request Jun 2, 2023
eugeneo pushed a commit to eugeneo/grpc that referenced this pull request Jun 7, 2023
Integration tests are passing, so we should be ready to
de-experimentalize.

Related: internal bug b/265209578
eugeneo pushed a commit to eugeneo/grpc that referenced this pull request Jun 7, 2023
Integration tests are passing, so we should be ready to
de-experimentalize.

Related: internal bug b/265209578
mario-vimal pushed a commit to mario-vimal/grpc that referenced this pull request Jun 15, 2023
Integration tests are passing, so we should be ready to
de-experimentalize.

Related: internal bug b/265209578
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/none imported Specifies if the PR has been imported to the internal repository lang/c++ lang/core per-call-memory/neutral per-channel-memory/neutral release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants