XDS: enable XDS federation by default #32711
Conversation
apolcyn
added
release notes: yes
src/core/ext/xds/xds_bootstrap.cc
Outdated
| @@ -29,7 +29,7 @@ namespace grpc_core { | |||
| // removed once federation is fully integrated and enabled by default. | |||
There was a problem hiding this comment.
Historically, in C-core, we've just completely removed the env var instead of changing the default to true, on the theory that once we pass interop tests, we have confidence that there won't be any more need to disable the feature. However, in this particular case, since we've already had a fairly serious bug with this code, it probably does make sense to be more cautious here, so I'm okay with this approach.
However, we need to remember to eventually come back and remove this completely, or else we'll just keep accumulating env var controls that we don't actually need. Please update the TODO here to indicate that this should be removed after the 1.55 release, just so that we have a target date for removing this.
There was a problem hiding this comment.
done, updated the todo
|
Manually tested the previously-failed tests - they should pass now. PTAL |
test/core/xds/xds_client_test.cc
Outdated
| TEST_F(XdsClientTest, FederationDisabledWithNewStyleName) { | ||
| // We will use this xdstp name, whose authority is not present in | ||
| // the bootstrap config. But since federation is not enabled, we | ||
| // will treat this as an opaque old-style name, so we'll send it to | ||
| // the default server. | ||
| grpc_core::testing::ScopedEnvVar env_var("GRPC_EXPERIMENTAL_XDS_FEDERATION", |
There was a problem hiding this comment.
Nit: Please move this up to line 2523, so it's not between the comment above and the line that the comment refers to.
|
Android failure |
copybara-service
bot
added
the
imported
This reverts commit 4b46dbc.
This reverts commit 4b46dbc.
This reverts commit 4b46dbc. Reason: this seems to be breaking load reports in certain cases, b/276944116 Let's revert so this doesn't accidentally get released.
…rpc#32814)" This reverts commit 4f189b1.
Integration tests have been green so let's enable this (verification of test results in https://b.corp.google.com/issues/262593165#comment30).
This reverts commit 4b46dbc. Reason: this seems to be breaking load reports in certain cases, b/276944116 Let's revert so this doesn't accidentally get released.
…)" (grpc#32814) (grpc#32902) Previous lack-of-load-reporting issue has been fixed (b/276944116)
Integration tests have been green so let's enable this (verification of test results in https://b.corp.google.com/issues/262593165#comment30).
Integration tests have been green so let's enable this (verification of test results in https://b.corp.google.com/issues/262593165#comment30).