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

Calculate initial concurrency based on resulting connection protocol #2693

Merged
merged 3 commits into from
Sep 14, 2023

Conversation

idelpivnitskiy
Copy link
Member

@idelpivnitskiy idelpivnitskiy commented Sep 14, 2023

Motivation:

RequestConcurrencyController is always applied as the top level wrapper of the connection (after all connection-level filters) before returning it back to the LoadBalancer. As the result, there is no need to make an earlier decision for initialConcurrency value at implementations of AbstractLBHttpConnectionFactory (when the final protocol may be unknown) because it can be inferred based on the resulting connection protocol at the time a new
ReservableRequestConcurrencyController is created. Early initialization as it's now makes it harder to switch protocols after proxy connect.

Modifications:

  • Convert abstract method AbstractLBHttpConnectionFactory.newConcurrencyController into a private method;
  • Compute initialConcurrency based on connection's protocol and ReadOnlyHttpClientConfig;

Result:

No need to pre-initialize initialConcurrency value in AbstractLBHttpConnectionFactory implementations as it will be computed based on the connection's protocol at the time it's required.

Motivation:

`RequestConcurrencyController` is always applied as the top level
wrapper of the connection (after all connection-level filters) before
returning it back to the `LoadBalancer`. As the result, there is no
need to make an earlier decision for `initialConcurrency` value at
`AbstractLBHttpConnectionFactory` implementation (when the final
protocol may be unknown) because it can be inferred based on the
resulting connection protocol at the time a new
`ReservableRequestConcurrencyController` is created. Early
initialization as it's now makes it harder to switch protocols after
proxy connect.

Modifications:

- Remove abstract method
`AbstractLBHttpConnectionFactory.newConcurrencyController`;
- Compute `initialConcurrency` based on connection's protocol and
`ReadOnlyHttpClientConfig`;
- Update `ReservableRequestConcurrencyControllersTest`;

Result:

No need to pre-initialize `initialConcurrency` value in
`AbstractLBHttpConnectionFactory` implementations as it will be computed
based on the connection's protocol at the time it's required.
@idelpivnitskiy
Copy link
Member Author

I'm trying to extract meaningful code changes into independent PRs before finalizing PR for a proxy connect with ALPN (can switch from HTTP/1.1 to HTTP/2 protocol)

@idelpivnitskiy
Copy link
Member Author

@daschl as we discussed offline, reverted changes for ReservableRequestConcurrencyControllers and move computation logic to AbstractLBHttpConnectionFactory. Now the delta looks smaller and ReservableRequestConcurrencyControllers doesn't need to know anything about ReadOnlyHttpClientConfig.

@idelpivnitskiy idelpivnitskiy merged commit 78d4050 into apple:main Sep 14, 2023
@idelpivnitskiy idelpivnitskiy deleted the initialConcurrency branch September 14, 2023 18:27
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.

2 participants