Make RequestConcurrencyController public
#2278
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: Users can not wrap connections inside the `LoadBalancer` or `HttpLoadBalancerFactory` because `RequestConcurrencyController` API is internal. Modifications: - Move `RequestConcurrencyController` and `ReservableRequestConcurrencyController` from `client-api-internal` to `client-api`; - `FilterableStreamingHttpLoadBalancedConnection` now extends `ReservableRequestConcurrencyController` and `ReservedStreamingHttpConnection`; - Introduce `HttpLoadBalancerFactory#toLoadBalancedConnection` overload that also accepts `ReservableRequestConcurrencyController`, deprecate pre-existing overload; - Add `DefaultFilterableStreamingHttpLoadBalancedConnection` that helps users combine `FilterableStreamingHttpConnection`, `ReservableRequestConcurrencyController` and `ScoreSupplier` or do wrapping; - Remove `LoadBalancedStreamingHttpConnection` as it's not required anymore; - Update internals to use new API; Result: Users can wrap the `FilterableStreamingHttpLoadBalancedConnection` inside their `LoadBalancer` or `HttpLoadBalancerFactory` implementations.
idelpivnitskiy
commented
Jul 18, 2022
| FilterableStreamingHttpConnection connection); | ||
| default FilterableStreamingHttpLoadBalancedConnection toLoadBalancedConnection( | ||
| FilterableStreamingHttpConnection connection, | ||
| ReservableRequestConcurrencyController concurrencyController) { |
There was a problem hiding this comment.
I'm open to discussions here. For now, I tried to keep pre-existing assumption that the HttpLoadBalancerFactory knows how to implement score() for the passed connection. In practice, the score() implementation may not be self-sufficient and can rely on external information from the LB or CF. Since we already pass the ContextMap through request -> selectConnection -> newConnection chain, we can add it here as a 3rd nullable argument too to preserve that context info. LMK WDYT.
There was a problem hiding this comment.
Discussed with @tkountis offline and agreed to add the context as the 3rd arg. It may be helpful to propagate information between layers without API changes in the future. Also, consistency with selectConnection and newConnection.
|
@tkountis your offline comments addressed, please have another look |
tkountis
approved these changes
Jul 29, 2022
|
Nice work. |
idelpivnitskiy
added a commit
to idelpivnitskiy/servicetalk
that referenced
this pull request
Aug 3, 2022
…le#2278 Motivation: apple#2278 moved `RequestConcurrencyController` and `ReservableRequestConcurrencyController` from `client-api-internal` to `client-api` module. This is an API incompatible change for 0.42.x. Also, it removed `DefaultHttpLoadBalancerFactory#toLoadBalancedConnection` impl, this is a binary incompatible change. Modifications: - Restore `io.servicetalk.client.api.internal.RequestConcurrencyController` and `io.servicetalk.client.api.internal.ReservableRequestConcurrencyController` interfaces and all references to them in `client-api-internal` module; - Create a pkg-private copy of `ReservableRequestConcurrencyControllers` inside `http-netty` module that targets public `io.servicetalk.client.api.ReservableRequestConcurrencyController`; - Copy tests for `ReservableRequestConcurrencyControllers`; - Create a pkg-private copy of `io.servicetalk.client.api.internal.IgnoreConsumedEvent` in `http-netty` module; - Deprecate all classes and interfaces in `client-api-internal` module; - Remove unnecessary references to `servicetalk-client-api-internal` dependency in modules that don't actually depend on it; Result: 1. The next 0.42.14 release will be backward compatible with 0.42.13. 2. The whole `servicetalk-client-api-internal` module is deprecated now and can be removed in the future releases. Signed-off-by: Idel Pivnitskiy <idel.pivnitskiy@apple.com>
idelpivnitskiy
added a commit
that referenced
this pull request
Aug 4, 2022
… (#2306) Motivation: #2278 moved `RequestConcurrencyController` and `ReservableRequestConcurrencyController` from `client-api-internal` to `client-api` module. This is an API incompatible change for 0.42.x. Also, it removed `DefaultHttpLoadBalancerFactory#toLoadBalancedConnection` impl, this is a binary incompatible change. Modifications: - Restore `io.servicetalk.client.api.internal.RequestConcurrencyController` and `io.servicetalk.client.api.internal.ReservableRequestConcurrencyController` interfaces and all references to them in `client-api-internal` module; - Create a pkg-private copy of `ReservableRequestConcurrencyControllers` inside `http-netty` module that targets public `io.servicetalk.client.api.ReservableRequestConcurrencyController`; - Copy tests for `ReservableRequestConcurrencyControllers`; - Create a pkg-private copy of `io.servicetalk.client.api.internal.IgnoreConsumedEvent` in `http-netty` module; - Deprecate all classes and interfaces in `client-api-internal` module; - Remove unnecessary references to `servicetalk-client-api-internal` dependency in modules that don't actually depend on it; Result: 1. The next 0.42.14 release will be backward compatible with 0.42.13. 2. The whole `servicetalk-client-api-internal` module is deprecated now and can be removed in future releases.
idelpivnitskiy
added a commit
to idelpivnitskiy/servicetalk
that referenced
this pull request
Aug 30, 2022
Motivation: apple#2278 added a new connection wrapper `DefaultFilterableStreamingHttpLoadBalancedConnection` that does not implement `toString()`. It hides the channel id in RRLB logs. Modifications: - Implement `toString()` for `DefaultFilterableStreamingHttpLoadBalancedConnection` in `HttpLoadBalancerFactory` and `DefaultHttpLoadBalancerFactory`; - Make `AbstractStreamingHttpConnection#toString()` final; Result: RRLB logs include channel id.
idelpivnitskiy
added a commit
that referenced
this pull request
Sep 8, 2022
Motivation: #2278 added a new connection wrapper `DefaultFilterableStreamingHttpLoadBalancedConnection` that does not implement `toString()`. It hides the channel id in RRLB logs. Modifications: - Implement `toString()` for `DefaultFilterableStreamingHttpLoadBalancedConnection` in `HttpLoadBalancerFactory` and `DefaultHttpLoadBalancerFactory`; - Make `AbstractStreamingHttpConnection#toString()` final; Result: RRLB logs include channel id.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Users can not wrap connections inside the
LoadBalancerorHttpLoadBalancerFactorybecauseRequestConcurrencyControllerAPI isinternal.
Modifications:
RequestConcurrencyControllerandReservableRequestConcurrencyControllerfromclient-api-internaltoclient-api;LoadBalancedConnectionnow extendsReservableRequestConcurrencyController;FilterableStreamingHttpLoadBalancedConnectionnow extendsReservedStreamingHttpConnection;HttpLoadBalancerFactory#toLoadBalancedConnectionoverloadthat also accepts
ReservableRequestConcurrencyControllerandContextMap;HttpLoadBalancerFactory#toLoadBalancedConnection(FilterableStreamingHttpConnection);DefaultFilterableStreamingHttpLoadBalancedConnectionthat helpsusers combine
FilterableStreamingHttpConnection,ReservableRequestConcurrencyControllerandScoreSupplieror dowrapping;
LoadBalancedStreamingHttpConnectionas it's not requiredanymore;
Result:
Users can wrap the
FilterableStreamingHttpLoadBalancedConnectioninside their
LoadBalancerorHttpLoadBalancerFactoryimplementations.