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

A49: Dynamic Connection Scaling #276

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Nov 22, 2021

A feature to automatically create new connections when the limit of streams on a
connection is reached.

A feature to automatically create new connections when the limit of streams on a
connection is reached.
@dfawley dfawley marked this pull request as ready for review November 22, 2021 19:57
@murgatroid99
Copy link
Member

If I understand this design right, the client needs to get a service config to enable this feature. In my experience, our users often encounter this limit when talking to GFE servers while using Google Cloud API client libraries, or similar. For those users, what do we expect to be the source of the service config?

@dfawley
Copy link
Member Author

dfawley commented Dec 3, 2021

Default service config (and optionally disabling name resolver service config) can also be injected by the application. Client libraries already does this, I believe, for other SC features. This is how we plan to enable it there.

@murgatroid99
Copy link
Member

If the expected use case is for users to set this option statically at channel creation time, why not make it just a channel argument?

@ejona86
Copy link
Member

ejona86 commented Dec 3, 2021

The service provider should be in control of the setting, which means service config is the ideal location.

Comment on lines +163 to +169
Subchannels may be shared across channels. The `maxConnectionsPerSubchannel`
setting will be applied to a shared subchannel and that subchannel will use the
highest maximum of all channels currently sharing it. If channels sharing the
subchannel are closed, then the remaining channels' settings will be combined to
determine the new max. If the max is lowered to less than the number of open
connections, the excess connections will not be closed as a result.
Down-scaling will only happen as connections are lost naturally.
Copy link
Member

Choose a reason for hiding this comment

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

I think this part can be simplified. In Node, and I think also Core, subchannels are only shared among channels with the same target and channel arguments. Two such channels should never have different service configs except in brief windows when updates may propagate to separate resolvers at slightly different times. So, there probably isn't much value in tracking which setting value is associated with which channel, and instead we can just use the latest value the subchannel has seen from any channel.

@murgatroid99
Copy link
Member

OK, then do any of the libraries already have a mechanism for propagating service config updates to subchannels?

// ...

// Settings to control dynamic connection scaling. For more details,
// please refer to gRFC AXX (this one; XX = TBD).
Copy link
Member

Choose a reason for hiding this comment

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

This part needs to use spaces and tabs consistently.

// ...

// Settings to control dynamic connection scaling. For more details,
// please refer to gRFC AXX (this one; XX = TBD).
Copy link
Member

Choose a reason for hiding this comment

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

You can fill in the gRFC number now.

// lowering this value. Down-scaling will only happen as
// connections are lost naturally.
//
// Must be a whole number greater than 1. Values higher than
Copy link
Member

Choose a reason for hiding this comment

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

We should probably allow 1 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants