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: Configure outlier detection. #9456

Merged
merged 2 commits into from Aug 18, 2022
Merged

Conversation

temawi
Copy link
Contributor

@temawi temawi commented Aug 18, 2022

Enables the new OutlierDetectionLoadBalancer when outlier detection is enabled
in the xDS cluster configuration.

@temawi temawi marked this pull request as ready for review August 18, 2022 18:14
@temawi
Copy link
Contributor Author

temawi commented Aug 18, 2022

cc:
@ejona86
@murgatroid99

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

I didn't notice any major issues.

xds/src/main/java/io/grpc/xds/ClientXdsClient.java Outdated Show resolved Hide resolved
xds/src/main/java/io/grpc/xds/EnvoyServerProtoData.java Outdated Show resolved Hide resolved
Copy link
Contributor

@larry-safran larry-safran left a comment

Choose a reason for hiding this comment

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

Test edsClustersWithOutlierDetection should be fixed. Using the constant intsead of string literal would be nice.

// load balancer.
if (outlierDetection != null) {
LoadBalancerProvider outlierDetectionProvider = lbRegistry.getProvider(
"outlier_detection_experimental");
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a constant like the rest of the policy names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There wasn't really a nice place to put it, since the constant file in the xds project is for xds load balancers - which this is not as it lives in core. There is a plan to move the load balancer over to this project though so we can introduce a constant at that point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. I would have put it in the OutlierDectionLoadBalanceProvider class itself, but waiting until later is fine.

@temawi temawi merged commit 81abb21 into grpc:master Aug 18, 2022
@temawi temawi deleted the outlier-detection-xds branch August 18, 2022 23:06
temawi added a commit to temawi/grpc-java that referenced this pull request Aug 22, 2022
Enables the new OutlierDetectionLoadBalancer when outlier detection is enabled
in the xDS cluster configuration.
temawi added a commit that referenced this pull request Aug 22, 2022
Enables the new OutlierDetectionLoadBalancer when outlier detection is enabled
in the xDS cluster configuration.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 17, 2022
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.

None yet

3 participants