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

HTTP2 - Round-robining streams across a minimum setting of connections #1808

Closed
crankydillo opened this issue Sep 14, 2021 · 15 comments
Closed
Assignees
Labels
type/enhancement A general enhancement
Milestone

Comments

@crankydillo
Copy link

We leverage L4 load balancers for distributing load, resiliency needs, etc. We are thinking 2 additions (likely opt-in) to the HTTP2 pool could help us in this regard:

  1. The HTTP2 pool maintains a minimum number of connections before opening additional streams.
  2. As it opens additional streams, it round-robins between the connections in the HTTP2 pool. A fancier technique where it chooses the conn with the least number of open streams is likely better, but it's unclear yet if that is critical.

Some of the above was initially discussed here.

@crankydillo crankydillo added status/need-triage A new issue that still need to be evaluated as a whole type/enhancement A general enhancement labels Sep 14, 2021
@violetagg
Copy link
Member

@crankydillo Can you elaborate more on this The HTTP2 pool maintains a minimum number of connections before opening additional streams.

@violetagg violetagg added for/user-attention This issue needs user attention (feedback, rework, etc...) and removed status/need-triage A new issue that still need to be evaluated as a whole labels Sep 14, 2021
@crankydillo
Copy link
Author

Currently, you only get a new connection from the underlying pool if requests are made in parallel in certain ways or if you have exhausted the max streams on a connection. In the latter case, that can result in max streams requests all getting sent to one server that sits behind an L4 load balancer. What we want is to specify at the client level that the some minimum number of connections should be used to round-robin the streams. With that, we get some client-side load-balancing of requests. We also close connections from the server and are assuming that graceful handling of that will happen in the client.

@crankydillo
Copy link
Author

@crankydillo Can you elaborate more on this The HTTP2 pool maintains a minimum number of connections before opening additional streams.

If the above doesn't help, my precise definition of that prhase is:

Up to some number (the minimum) the client will open a new connection when creating a new stream as opposed to creating an additional stream on a connection that is already in the HTTP/2 pool.

@violetagg violetagg self-assigned this Sep 21, 2021
@violetagg violetagg removed the for/user-attention This issue needs user attention (feedback, rework, etc...) label Sep 21, 2021
@violetagg violetagg added this to the 1.0.12 milestone Sep 21, 2021
@crankydillo
Copy link
Author

crankydillo commented Sep 30, 2021

@violetagg There is another aspect of this that we need to get clarity on. We are hoping that new (socket) connections will be gated by this minimum pool until min-pool size * max streams per conn is reached.

For example, if we have:

HttpClient client = Http.builderStuff().maxConnections(256)).build();
final int concurrency = 100;
Flux.range(0, N)  // N often, but not guaranteed to, exceed concurrency level
   .flatMap(i -> clientCall(client), concurrency)
   .subscribe();

We will get ~100 socket connections created because of the concurrency level in flatMap. However, we'd like to guarantee that socket connections will remain below or equal to the min-pool size feature you are working in this feature request until min-pool size * max_active_streams has been reached, while attempting to concurrently make calls. Without this gate, doesn't it imply that you must have N > concurrency level in order to get any benefit from HTTP/2 streams?

We feel like this min pool feature is related to this and apologize for throwing this in now. What do you think? Do you agree with our concern above?

@violetagg violetagg modified the milestones: 1.0.12, 1.0.13 Oct 7, 2021
@violetagg violetagg modified the milestones: 1.0.13, 1.0.14 Nov 3, 2021
@violetagg violetagg modified the milestones: 1.0.14, 1.0.15 Dec 6, 2021
@violetagg violetagg modified the milestones: 1.0.15, 1.0.x Backlog Dec 22, 2021
@violetagg violetagg removed their assignment Dec 23, 2021
@crankydillo
Copy link
Author

crankydillo commented Mar 8, 2022

Hello. Just wanted to touch base on this. It looks like it's not a high priority for you, but it currently still is for us. What do you think about us making a contribution? Overly difficult? We know reactor-pool has a minimum-resource type of concept. I tried a quick hack to make the underlying pool have a minimum size (even though I know you have checks like this), but it failed. Is that a bad route?

We can poke around more, but if you have any suggestions, we welcome them:) Also, if you don't plan on dealing with any PRs related to this feature in the near term, that would be good information for us to know as well.

Many thanks for all the hard work!

@violetagg
Copy link
Member

violetagg commented Mar 8, 2022

@crankydillo Let me schedule this for 1.0.18. We won't be able to work on this for 1.0.17 (release is next week). Is that OK?
I'll take a look at the provided link later this week.

@violetagg violetagg modified the milestones: 1.0.x Backlog, 1.0.18 Mar 8, 2022
@crankydillo
Copy link
Author

Thank you Violeta. I'm pretty sure I know the answer, but just in case.. Will you be able to consider a PR targeting 1.0.17?

@violetagg
Copy link
Member

Thank you Violeta. I'm pretty sure I know the answer, but just in case.. Will you be able to consider a PR targeting 1.0.17?

I'll be able to review a PR

crankydillo pushed a commit to crankydillo/reactor-netty that referenced this issue Mar 9, 2022
before spawning multiple streams on any given socket connection.

 reactor#1808
@violetagg violetagg modified the milestones: 1.0.18, 1.0.x Backlog Apr 4, 2022
@crankydillo
Copy link
Author

crankydillo commented Apr 4, 2022

@violetagg There is another aspect of this that we need to get clarity on. We are hoping that new (socket) connections will be gated by this minimum pool until min-pool size * max streams per conn is reached.

For example, if we have:

HttpClient client = Http.builderStuff().maxConnections(256)).build();
final int concurrency = 100;
Flux.range(0, N)  // N often, but not guaranteed to, exceed concurrency level
   .flatMap(i -> clientCall(client), concurrency)
   .subscribe();

We will get ~100 socket connections created because of the concurrency level in flatMap. However, we'd like to guarantee that socket connections will remain below or equal to the min-pool size feature you are working in this feature request until min-pool size * max_active_streams has been reached, while attempting to concurrently make calls. Without this gate, doesn't it imply that you must have N > concurrency level in order to get any benefit from HTTP/2 streams?

We feel like this min pool feature is related to this and apologize for throwing this in now. What do you think? Do you agree with our concern above?

@violetagg You asked on the PR what the requirements are. I'm trying to get that written up nicely. The main requirement remains that a specifiable minimal number of socket connections are created before any additional streams are created on an existing socket connection. However, what do you think about the scenario above?

@violetagg
Copy link
Member

Update: The exact requirements: #2091 (comment)

  • Some configurable, minimum number of socket connections are created before ever creating more than one stream on any given socket connection.

@crankydillo For the next release which is next Tuesday (12.04) I will be able to expose a configuration for specifying a minimum number of connections and a warmup that you have to call.
The part for the streams I can schedule for the next release.
Wdyt?

  • When creating streams, choosing a socket connection is done in a round-robin fashion.

We do use round-robin as of today. I cannot understand this requirement.

@violetagg
Copy link
Member

@crankydillo PTAL #2155

@crankydillo
Copy link
Author

Thanks @violetagg . We plan on looking at your PR soon!

@violetagg violetagg modified the milestones: 1.0.19, 1.0.20 May 3, 2022
@violetagg violetagg linked a pull request Jun 8, 2022 that will close this issue
@violetagg
Copy link
Member

The enhancement is available in 1.0.20-SNAPSHOT. It will be great if you can test it.

The min connections can be configured via the new Http2AllocationStrategy. See an example below:

ConnectionProvider provider = ConnectionProvider.builder("test")
        .allocationStrategy(Http2AllocationStrategy.builder().maxConcurrentStreams(1000).minConnections(5).maxConnections(500).build())
        .build();

@crankydillo
Copy link
Author

@violetagg I'm sorry I missed this. I know it's quite late, but I will try this out!

@crankydillo
Copy link
Author

I tried it out. Pretty sure it's going to work out like we hoped. Thanks for implementing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A general enhancement
Projects
None yet
2 participants