-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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: enable Outlier Detection by default #5673
Conversation
39ae0b2
to
3109845
Compare
3109845
to
ea476a5
Compare
// "GRPC_EXPERIMENTAL_ENABLE_OUTLIER_DETECTION" to "true". | ||
XDSOutlierDetection = strings.EqualFold(os.Getenv(outlierDetectionSupportEnv), "true") | ||
// enabled, which can be disabled by setting the environment variable | ||
// "GRPC_EXPERIMENTAL_ENABLE_OUTLIER_DETECTION" to "false". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: We haven't done this in the past, but at some point we discussed this. We should add the release that we are enabling it from, and add a TODO saying at what release we should completely get rid of the env var. Without this, we will have an ever growing list of env vars here and sprinkled everywhere else in the code.
func TestBuildPriorityConfig(t *testing.T) { | ||
gotConfig, gotAddrs, _ := buildPriorityConfig([]priorityConfig{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we retain this test and run it with the env var set to "false"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with removing it given we would like to eliminate the env var entirely in a few weeks.
func TestBuildPriorityConfig(t *testing.T) { | ||
gotConfig, gotAddrs, _ := buildPriorityConfig([]priorityConfig{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with removing it given we would like to eliminate the env var entirely in a few weeks.
RELEASE NOTES: