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

Fix InstantiatingGrpcChannelProvider's channel pool to play nicely with DirectPath #798

Merged
merged 5 commits into from Oct 16, 2019

Conversation

igorbernstein2
Copy link
Collaborator

@igorbernstein2 igorbernstein2 commented Oct 7, 2019

// cc @WeiranFang

By default grpclb strategy for DirectPath is to create a subchannel for
every address resolved by the grpclb and round robin over them.
Unfortunately this doesn't work well when Channel pooling is enable in
the InstantiatingGrpcChannelProvider, which will create multiple
ManagedChannels, each containing a bunch of subchannels.

Since channel pooling is needed to have good performance targeting CFEs,
the solution here is to force each ManagedChannel to pick a single
subchannel. Thus preserving the CFE behavior of a single ManagedChannel
only containing a single subchannel.

Unfortunately I couldn't figure out a reasonable way to write tests for this. ManagedChannelBuilder nor ManagedChannel provide a way from me to introspect what the current service config is, so a unit test w/o reflection is not possible. And bringing up a direct path like environment for a functional test is a bit too much.

…th DirectPath

By default grpclb strategy for DirectPath is to create a subchannel for
every address resolved by the grpclb and round robin over them.
Unfortunately this doesn't work well when Channel pooling is enable in
the InstantiatingGrpcChannelProvider, which will create multiple
ManagedChannels, each containing a bunch of subchannels.

Since channel pooling is needed to have good performance targeting CFEs,
the solution here is to force each ManagedChannel to pick a single
subchannel. Thus preserving the CFE behavior of a single ManagedChannel
only containing a single subchannel.
@igorbernstein2 igorbernstein2 requested a review from chingor13 Oct 7, 2019
@googlebot googlebot added the cla: yes label Oct 7, 2019
@codecov
Copy link

@codecov codecov bot commented Oct 7, 2019

Codecov Report

Merging #798 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #798      +/-   ##
============================================
+ Coverage     78.39%   78.43%   +0.04%     
  Complexity     1106     1106              
============================================
  Files           198      198              
  Lines          4887     4897      +10     
  Branches        385      385              
============================================
+ Hits           3831     3841      +10     
  Misses          887      887              
  Partials        169      169
Impacted Files Coverage Δ Complexity Δ
...api/gax/grpc/InstantiatingGrpcChannelProvider.java 79.44% <100%> (+1.2%) 36 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 664f671...4ed3069. Read the comment docs.

@chingor13
Copy link
Contributor

@chingor13 chingor13 commented Oct 9, 2019

Is there a link to the documentation somewhere about the configuration we're specifying? I couldn't find much on the ForwardingChannelBuilder docs. The map of list of maps looks pretty arbitrary at first glance.

@igorbernstein2
Copy link
Collaborator Author

@igorbernstein2 igorbernstein2 commented Oct 9, 2019

Maybe @WeiranFang can comment on the service config semantics? I don't know of any docs for the service config format. The config is the translation of the serverside config:
{"loadBalancingConfig":[{"grpclb":{"childPolicy":[{"pick_first":{}}]}}]}

See cl/243322471, for the original source. I reformatted the code and introduced local variables to make it easier to read. But unfortunately I can't offer more info on the semantics

@WeiranFang
Copy link
Contributor

@WeiranFang WeiranFang commented Oct 13, 2019

Hi @chingor13 , the usage of this defaultServiceConfig is defined in ManagedChannelBuilder. And the implementation is in AbstractManagedChannelImplBuilder.

I also found the spec for adding this API.

As for the semantics of service config, check the GrpcLbConfig defined in service_config.proto

Thanks!

@igorbernstein2
Copy link
Collaborator Author

@igorbernstein2 igorbernstein2 commented Oct 14, 2019

@chingor13 PTAL

@igorbernstein2 igorbernstein2 merged commit 778c8e3 into googleapis:master Oct 16, 2019
5 checks passed
@igorbernstein2 igorbernstein2 deleted the dp-pool branch Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants