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: Use weighted_target LB provider in wrr_locality #9195

Merged
merged 1 commit into from May 24, 2022

Conversation

temawi
Copy link
Contributor

@temawi temawi commented May 24, 2022

Fixes a bug where WrrLocalityLoadBalancer would use the endpoint picking policy provider instead of WeightedTargetLoadBalancerProvider.

Also adds a test to fake control plane integration test that caught this bug. The test scaffolding is also updated to have the test server echo all client headers back in the response.

The test load balancer in the test is an almost straight copy of: https://github.com/grpc/grpc-java/blob/master/interop-testing/src/test/java/io/grpc/testing/integration/RpcBehaviorLoadBalancerProviderTest.java

@temawi temawi force-pushed the wrr_locality_use_weighted_target branch from 9b4b11f to 828dd02 Compare May 24, 2022 16:51
Copy link
Contributor

@YifeiZhuang YifeiZhuang left a comment

Choose a reason for hiding this comment

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

LGTM. I'm glad the test infra is useful.

@Override
public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(ServerCall<ReqT, RespT> call,
Metadata requestHeaders, ServerCallHandler<ReqT, RespT> next) {
logger.info("Received following metadata: " + requestHeaders);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: lower the log level to fine is better to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Fixes a bug where WrrLocalityLoadBalancer would use the endpoint picking policy provider instead of WeightedTargetLoadBalancerProvider.

Also adds a test to fake control plane integration test that caught this bug. The test scaffolding is also updated to have the test server echo all client headers back in the response.

The test load balancer in the test is an almost straight copy of: https://github.com/grpc/grpc-java/blob/master/interop-testing/src/test/java/io/grpc/testing/integration/RpcBehaviorLoadBalancerProviderTest.java
@temawi temawi force-pushed the wrr_locality_use_weighted_target branch from 828dd02 to 8422cc1 Compare May 24, 2022 20:03
@temawi temawi merged commit 48ea07e into grpc:master May 24, 2022
@temawi temawi deleted the wrr_locality_use_weighted_target branch May 24, 2022 20:51
@temawi temawi added the TODO:backport PR needs to be backported. Removed after backport complete label May 26, 2022
temawi added a commit to temawi/grpc-java that referenced this pull request May 27, 2022
Fixes a bug where WrrLocalityLoadBalancer would use the endpoint picking policy provider instead of WeightedTargetLoadBalancerProvider.

Also adds a test to fake control plane integration test that caught this bug. The test scaffolding is also updated to have the test server echo all client headers back in the response.

The test load balancer in the test is an almost straight copy of: https://github.com/grpc/grpc-java/blob/master/interop-testing/src/test/java/io/grpc/testing/integration/RpcBehaviorLoadBalancerProviderTest.java
temawi added a commit that referenced this pull request May 27, 2022
Fixes a bug where WrrLocalityLoadBalancer would use the endpoint picking policy provider instead of WeightedTargetLoadBalancerProvider.

Also adds a test to fake control plane integration test that caught this bug. The test scaffolding is also updated to have the test server echo all client headers back in the response.

The test load balancer in the test is an almost straight copy of: https://github.com/grpc/grpc-java/blob/master/interop-testing/src/test/java/io/grpc/testing/integration/RpcBehaviorLoadBalancerProviderTest.java
@temawi temawi removed the TODO:backport PR needs to be backported. Removed after backport complete label May 27, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 26, 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

2 participants