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

interop-testing: LB that adds the rpc-behavior to calls #9186

Merged
merged 3 commits into from
May 20, 2022

Conversation

temawi
Copy link
Contributor

@temawi temawi commented May 18, 2022

This load balancer can be used to trigger custom behavior on servers for all calls that use the LB.

@temawi temawi requested a review from YifeiZhuang May 18, 2022 22:48
@@ -0,0 +1 @@
io.grpc.testing.integration.RpcBehaviorLoadBalancerProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to use it to register this provider to the LoadBalancerRegistry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes exactly, this file will be in the classpath and read automatically to register any LB providers in it.

Copy link
Contributor

Choose a reason for hiding this comment

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

yea ok, is it possible to call register before test and de-register after? I strongly feel that our test provider should better not mix with production service providers. But I don't know how bad it is. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my initial plan, but @ejona86 suggested this approach. Eric, would our interop-testing project META-INF ever mingle with the real one(s)?

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't mix with our production service providers because it is part of interop-testing. I don't think we should register this per-test, because that's not at all how the test architecture works, which is also why we aren't doing that for Ivy's interop tests. The xds interop tests don't know what test they are running, so they can't at all register per-test (vs just having hacks). We'd have to add a new argument for the binary and I don't think that really provides any value.

private static final String RPC_BEHAVIOR_HEADER_KEY = "rpc-behavior";

private final SubchannelPicker delegatePicker;
private final String rpcBehavior;
Copy link
Contributor

Choose a reason for hiding this comment

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

if i understand correctly, the picker is created in managedChannel syncContext, while pick subchannel is called from created client stream, so volatile would be ideal here, right? cc. @ejona86

Copy link
Member

Choose a reason for hiding this comment

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

You are right, except for the volatile part. The picker only needs to worry about synchronizing with itself, and it isn't changing rpcBehavior, so everything is okay.

@@ -0,0 +1 @@
io.grpc.testing.integration.RpcBehaviorLoadBalancerProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

yea ok, is it possible to call register before test and de-register after? I strongly feel that our test provider should better not mix with production service providers. But I don't know how bad it is. wdyt?

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

@temawi temawi merged commit 4a5f6ad into grpc:master May 20, 2022
@temawi temawi deleted the test-custom-lb branch May 20, 2022 19:18
@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
@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

3 participants