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

MultiAddressClientBuilder execution strategy control #2166

Merged
merged 64 commits into from
Jun 15, 2022
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
64 commits
Select commit Hold shift + click to select a range
dc260f4
`MultiAddressClientBuilder` execution strategy control
bondolo Jan 21, 2022
20cec2a
fixes ClientEffectiveStrategyTest for basic Single and Multi builders
bondolo Apr 1, 2022
65ceda1
fixes ClientEffectiveStrategyTest for multi builder with initializer
bondolo Apr 1, 2022
61814d1
pre-final cleanup
bondolo Apr 1, 2022
09e2ea9
documentation improvements
bondolo Apr 1, 2022
db7f892
Handle single address builder overrides of offloadNone()
bondolo Apr 7, 2022
6392624
checkstyle nits
bondolo Apr 7, 2022
24d6216
combine client api test cases
bondolo Apr 7, 2022
dd7a88d
review feedback
bondolo Apr 13, 2022
074584a
remove DefaultClientGroup changes from PR
bondolo Apr 13, 2022
2be80b6
test cleanup
bondolo Apr 13, 2022
b2dbc55
Merge branch 'main' into multiaddress-initializer-strategy
bondolo Apr 14, 2022
4ade1dc
move FilterableClientToClient back
bondolo Apr 14, 2022
b0d2c3e
additional review feedback
bondolo Apr 15, 2022
0f7c444
test cleanup
bondolo Apr 15, 2022
3e151d6
test cleanup
bondolo Apr 15, 2022
035fdd3
fix multi-address builder strategy computation
bondolo Apr 15, 2022
030833a
checkstyle nit
bondolo Apr 15, 2022
5cb3a92
more test simplification
bondolo Apr 15, 2022
efba308
even more review feedback
bondolo Apr 15, 2022
e541a50
Merge branch 'main' into multiaddress-initializer-strategy
bondolo Apr 18, 2022
47ec7f8
test improvements and suggestions from Idel's review
bondolo Apr 18, 2022
8f0819d
allow send offload to execute on application thread
bondolo Apr 19, 2022
330824e
allow send offload to execute on application thread
bondolo Apr 19, 2022
97453d9
restore second request
bondolo Apr 19, 2022
c7d00bd
enhance visibility in tests
idelpivnitskiy Apr 20, 2022
522e7e6
more debugging
idelpivnitskiy Apr 20, 2022
36def49
more debugging 2
idelpivnitskiy Apr 20, 2022
9e6530d
add timestamps
idelpivnitskiy Apr 21, 2022
90d1728
Merge branch 'apple:main' into multiaddress-initializer-strategy
bondolo Apr 21, 2022
2c7e1df
change order or ClientApi
idelpivnitskiy Apr 21, 2022
5d95094
only allow application thread if not offloading
bondolo Apr 20, 2022
be76acf
use local for simplification
bondolo Apr 21, 2022
1419244
revert unintended changes
bondolo Apr 21, 2022
60040b2
capture where context map created and assigned
idelpivnitskiy Apr 22, 2022
efaa5a5
capture requestContext id too
idelpivnitskiy Apr 22, 2022
9d6e6a4
Remove ExecutionMode.CONCURRENT
idelpivnitskiy Apr 22, 2022
ef49a05
Revert intermediate debugging
idelpivnitskiy Apr 23, 2022
d670415
Remove intermediate debug logging
idelpivnitskiy Apr 23, 2022
a33671b
reorder client api
idelpivnitskiy Apr 23, 2022
312c836
revert `ExecutionMode.CONCURRENT`
idelpivnitskiy Apr 23, 2022
34a468d
small refactoring
idelpivnitskiy Apr 23, 2022
3079781
small refactoring 2
idelpivnitskiy Apr 23, 2022
f591a1c
small refactoring 3
idelpivnitskiy Apr 23, 2022
79b922c
rename filter, add javadoc to clarify its behavior
idelpivnitskiy Apr 23, 2022
da80a7e
Add assertion checking for conversion reducing offloading
bondolo Apr 25, 2022
b45b45d
improve executionStrategy() documentation
bondolo Apr 27, 2022
09e63bd
improved javadoc
bondolo Apr 29, 2022
d5a83f0
documentation refinement for discussion
bondolo May 18, 2022
acd2535
Merge branch 'main' into multiaddress-initializer-strategy
bondolo Jun 6, 2022
dfa383b
Merge branch 'main' into multiaddress-initializer-strategy
bondolo Jun 7, 2022
8380d2a
Merge branch 'main' into multiaddress-initializer-strategy
bondolo Jun 7, 2022
cead445
Merge branch 'main' into multiaddress-initializer-strategy
bondolo Jun 8, 2022
945762a
Merge branch 'main' into multiaddress-initializer-strategy
bondolo Jun 8, 2022
10da51b
Merge branch 'main' into multiaddress-initializer-strategy
bondolo Jun 9, 2022
7652859
Use current strategy for merging with offloadNone()
bondolo Jun 9, 2022
c52950c
adjust everything to current "builder takes precedece" rule
bondolo Jun 10, 2022
aebccc9
documentation matches behaviour and fix multi-offloadNone overrid
bondolo Jun 10, 2022
55c5095
fix javadoc nit
bondolo Jun 10, 2022
239b3aa
remove redundant modification of the request execution strategy
bondolo Jun 10, 2022
b6a4c15
checkstyle nit
bondolo Jun 10, 2022
c385897
remove assertion of incompatible behavior
bondolo Jun 15, 2022
81f818f
Merge branch 'main' into multiaddress-initializer-strategy
bondolo Jun 15, 2022
ee1ba16
final review feedback
bondolo Jun 15, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ enum DefaultHttpExecutionStrategy implements HttpExecutionStrategy {
OFFLOAD_SEND_EVENT_STRATEGY(EnumSet.of(OFFLOAD_SEND, OFFLOAD_EVENT)),
OFFLOAD_RECEIVE_META_AND_SEND_EVENT_STRATEGY(EnumSet.of(OFFLOAD_RECEIVE_META, OFFLOAD_SEND, OFFLOAD_EVENT)),
OFFLOAD_RECEIVE_DATA_AND_SEND_EVENT_STRATEGY(EnumSet.of(OFFLOAD_RECEIVE_DATA, OFFLOAD_SEND, OFFLOAD_EVENT)),
OFFLOADD_ALL_REQRESP_EVENT_STRATEGY(
OFFLOAD_ALL_REQRESP_EVENT_STRATEGY(
EnumSet.of(OFFLOAD_RECEIVE_META, OFFLOAD_RECEIVE_DATA, OFFLOAD_SEND, OFFLOAD_EVENT)),

OFFLOAD_CLOSE_STRATEGY(EnumSet.of(OFFLOAD_CLOSE)),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ private HttpApiConversions() {
* @return The conversion result.
* @deprecated Use overload with {@link HttpExecutionStrategy} rather than {@link HttpExecutionStrategyInfluencer}
*/
// FIXME: 0.43 - remove deprecated method
@Deprecated
public static ReservedHttpConnection toReservedConnection(ReservedStreamingHttpConnection original,
HttpExecutionStrategyInfluencer influencer) {
Expand Down Expand Up @@ -66,6 +67,7 @@ public static ReservedHttpConnection toReservedConnection(ReservedStreamingHttpC
* @return The conversion result.
* @deprecated Use overload with {@link HttpExecutionStrategy} rather than {@link HttpExecutionStrategyInfluencer}
*/
// FIXME: 0.43 - remove deprecated method
@Deprecated
public static ReservedBlockingHttpConnection toReservedBlockingConnection(
ReservedStreamingHttpConnection original, HttpExecutionStrategyInfluencer influencer) {
Expand Down Expand Up @@ -94,6 +96,7 @@ public static ReservedBlockingHttpConnection toReservedBlockingConnection(
* @return The conversion result.
* @deprecated Use overload with {@link HttpExecutionStrategy} rather than {@link HttpExecutionStrategyInfluencer}
*/
// FIXME: 0.43 - remove deprecated method
@Deprecated
public static ReservedBlockingStreamingHttpConnection toReservedBlockingStreamingConnection(
ReservedStreamingHttpConnection original, HttpExecutionStrategyInfluencer influencer) {
Expand Down Expand Up @@ -122,6 +125,7 @@ public static ReservedBlockingStreamingHttpConnection toReservedBlockingStreamin
* @return The conversion result.
* @deprecated Use overload with {@link HttpExecutionStrategy} rather than {@link HttpExecutionStrategyInfluencer}
*/
// FIXME: 0.43 - remove deprecated method
@Deprecated
public static HttpConnection toConnection(StreamingHttpConnection original,
HttpExecutionStrategyInfluencer influencer) {
Expand All @@ -148,6 +152,7 @@ public static HttpConnection toConnection(StreamingHttpConnection original, Http
* @return The conversion result.
* @deprecated Use overload with {@link HttpExecutionStrategy} rather than {@link HttpExecutionStrategyInfluencer}
*/
// FIXME: 0.43 - remove deprecated method
@Deprecated
public static BlockingHttpConnection toBlockingConnection(StreamingHttpConnection original,
HttpExecutionStrategyInfluencer influencer) {
Expand Down Expand Up @@ -175,6 +180,7 @@ public static BlockingHttpConnection toBlockingConnection(StreamingHttpConnectio
* @return The conversion result.
* @deprecated Use overload with {@link HttpExecutionStrategy} rather than {@link HttpExecutionStrategyInfluencer}
*/
// FIXME: 0.43 - remove deprecated method
@Deprecated
public static BlockingStreamingHttpConnection toBlockingStreamingConnection(
StreamingHttpConnection original, HttpExecutionStrategyInfluencer influencer) {
Expand Down Expand Up @@ -203,6 +209,7 @@ public static BlockingStreamingHttpConnection toBlockingStreamingConnection(
* @return The conversion result.
* @deprecated Use overload with {@link HttpExecutionStrategy} rather than {@link HttpExecutionStrategyInfluencer}
*/
// FIXME: 0.43 - remove deprecated method
@Deprecated
public static HttpClient toClient(StreamingHttpClient original, HttpExecutionStrategyInfluencer influencer) {
return toClient(original, influencer.requiredOffloads());
Expand All @@ -228,6 +235,7 @@ public static HttpClient toClient(StreamingHttpClient original, HttpExecutionStr
* @return The conversion result.
* @deprecated Use overload with {@link HttpExecutionStrategy} rather than {@link HttpExecutionStrategyInfluencer}
*/
// FIXME: 0.43 - remove deprecated method
@Deprecated
public static BlockingHttpClient toBlockingClient(StreamingHttpClient original,
HttpExecutionStrategyInfluencer influencer) {
Expand All @@ -254,6 +262,7 @@ public static BlockingHttpClient toBlockingClient(StreamingHttpClient original,
* @return The conversion result.
* @deprecated Use overload with {@link HttpExecutionStrategy} rather than {@link HttpExecutionStrategyInfluencer}
*/
// FIXME: 0.43 - remove deprecated method
@Deprecated
public static BlockingStreamingHttpClient toBlockingStreamingClient(StreamingHttpClient original,
HttpExecutionStrategyInfluencer influencer) {
Expand Down Expand Up @@ -281,6 +290,7 @@ public static BlockingStreamingHttpClient toBlockingStreamingClient(StreamingHtt
* @return {@link ServiceAdapterHolder} containing the service adapted to the streaming programming model.
* @deprecated Use overload with {@link HttpExecutionStrategy} rather than {@link HttpExecutionStrategyInfluencer}
*/
// FIXME: 0.43 - remove deprecated method
@Deprecated
public static ServiceAdapterHolder toStreamingHttpService(HttpService service,
HttpExecutionStrategyInfluencer influencer) {
Expand All @@ -307,6 +317,7 @@ public static ServiceAdapterHolder toStreamingHttpService(HttpService service, H
* @return {@link ServiceAdapterHolder} containing the service adapted to the streaming programming model.
* @deprecated Use overload with {@link HttpExecutionStrategy} rather than {@link HttpExecutionStrategyInfluencer}
*/
// FIXME: 0.43 - remove deprecated method
@Deprecated
public static ServiceAdapterHolder toStreamingHttpService(BlockingStreamingHttpService service,
HttpExecutionStrategyInfluencer influencer) {
Expand Down Expand Up @@ -334,6 +345,7 @@ public static ServiceAdapterHolder toStreamingHttpService(BlockingStreamingHttpS
* @return {@link ServiceAdapterHolder} containing the service adapted to the streaming programming model.
* @deprecated Use overload with {@link HttpExecutionStrategy} rather than {@link HttpExecutionStrategyInfluencer}
*/
// FIXME: 0.43 - remove deprecated method
@Deprecated
public static ServiceAdapterHolder toStreamingHttpService(BlockingHttpService service,
HttpExecutionStrategyInfluencer influencer) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,31 +30,31 @@
interface HttpClientBuilder<U, R, SDE extends ServiceDiscovererEvent<R>> {

/**
* Sets the {@link Executor} for all connections created from this builder.
* Sets the {@link Executor} for all clients created from this builder.
*
* @param executor {@link IoExecutor} to use.
* @return {@code this}.
*/
HttpClientBuilder<U, R, SDE> executor(Executor executor);

/**
* Sets the {@link IoExecutor} for all connections created from this builder.
* Sets the {@link IoExecutor} for all clients created from this builder.
*
* @param ioExecutor {@link IoExecutor} to use.
* @return {@code this}.
*/
HttpClientBuilder<U, R, SDE> ioExecutor(IoExecutor ioExecutor);

/**
* Sets the {@link BufferAllocator} for all connections created from this builder.
* Sets the {@link BufferAllocator} for all clients created from this builder.
*
* @param allocator {@link BufferAllocator} to use.
* @return {@code this}.
*/
HttpClientBuilder<U, R, SDE> bufferAllocator(BufferAllocator allocator);

/**
* Sets the {@link HttpExecutionStrategy} for all connections created from this builder.
* Sets the {@link HttpExecutionStrategy} for all clients created from this builder.
*
* @param strategy {@link HttpExecutionStrategy} to use.
* @return {@code this}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,34 @@ default SingleAddressInitializer<U, R> append(SingleAddressInitializer<U, R> toA
@Override
MultiAddressHttpClientBuilder<U, R> executor(Executor executor);

/**
* {@inheritDoc}
*
* <p>Provides the base execution strategy for all clients created from this builder and the default strategy for
* the {@link SingleAddressHttpClientBuilder} used to construct client instances. The
* {@link #initializer(SingleAddressInitializer)} may be used for some customization of the execution strategy for a
* specific client. Unless {@link HttpExecutionStrategies#offloadNone()} is specified as the execution strategy on
* this builder then the single client computed execution strategy will be merged with this builder strategy.
*
* <dl>
* <dt>unspecified or {@link HttpExecutionStrategies#defaultStrategy()}
* <dd>Effective execution strategy will be appropriate for the API (async/blocking streaming/aggregate) of the
* client used and will be be merged with the computed execution strategy of the single address client produced
* by {@link SingleAddressHttpClientBuilder}.
*
*
* <dt>{@link HttpExecutionStrategies#offloadNone()}
* (or deprecated {@link HttpExecutionStrategies#offloadNever()}),
* a custom execution strategy ({@link HttpExecutionStrategies#customStrategyBuilder()}), or
* {@link HttpExecutionStrategies#offloadAll()}
* <dd>Will be used, as specified, without regard to the API of the client used and will be merged with the
* computed execution strategy of the single address client produced by
* {@link SingleAddressHttpClientBuilder}.
* </dl>
*
* @param strategy {@inheritDoc}
* @return {@inheritDoc}
*/
@Override
MultiAddressHttpClientBuilder<U, R> executionStrategy(HttpExecutionStrategy strategy);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,34 @@ SingleAddressHttpClientBuilder<U, R> appendConnectionFilter(
@Override
SingleAddressHttpClientBuilder<U, R> executor(Executor executor);

/**
* {@inheritDoc}
*
* <p>Unless {@link HttpExecutionStrategies#offloadNone()} is specified as the execution strategy on
* this builder, the actual execution strategy used will be influenced by the execution strategy required by filters
* added via {@link #appendClientFilter(StreamingHttpClientFilterFactory)},
* {@link #appendConnectionFilter(StreamingHttpConnectionFilterFactory)}, and
* {@link #appendConnectionFactoryFilter(ConnectionFactoryFilter)}</p>, etc.
*
* <dl>
* <dt>unspecified or {@link HttpExecutionStrategies#defaultStrategy()}
* <dd>Effective execution strategy will be appropriate for the API (async/blocking streaming/aggregate) of the
* client used and will be {@linkplain HttpExecutionStrategyInfluencer influenced} by required strategies of
* any filters.
*
* <dt>{@link HttpExecutionStrategies#offloadNone()}
* (or deprecated {@link HttpExecutionStrategies#offloadNever()})
* <dd>No offloading will be used regardless of the client API used or the influence of the filters.
bondolo marked this conversation as resolved.
Show resolved Hide resolved
*
* <dt>A custom execution strategy ({@link HttpExecutionStrategies#customStrategyBuilder()}) or
* {@link HttpExecutionStrategies#offloadAll()}
* <dd>Will be used, as specified, without regard to the API of the client used. Effective execution strategy
* will be specified value plus any additional offloads required by any filters.
* </dl>
*
* @param strategy {@inheritDoc}
* @return {@inheritDoc}
*/
@Override
SingleAddressHttpClientBuilder<U, R> executionStrategy(HttpExecutionStrategy strategy);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@ final class StreamingHttpClientToBlockingHttpClient implements BlockingHttpClien
private final HttpRequestResponseFactory reqRespFactory;

StreamingHttpClientToBlockingHttpClient(final StreamingHttpClient client, final HttpExecutionStrategy strategy) {
this.strategy = defaultStrategy() == strategy ?
DEFAULT_BLOCKING_CONNECTION_STRATEGY : strategy;
this.strategy = defaultStrategy() == strategy ? DEFAULT_BLOCKING_CONNECTION_STRATEGY : strategy;
this.client = client;
context = new DelegatingHttpExecutionContext(client.executionContext()) {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import static io.servicetalk.http.api.HttpContextKeys.HTTP_EXECUTION_STRATEGY_KEY;
import static io.servicetalk.http.api.HttpExecutionStrategies.defaultStrategy;
import static io.servicetalk.http.api.RequestResponseFactories.toAggregated;
import static io.servicetalk.http.api.StreamingHttpConnectionToHttpConnection.DEFAULT_CONNECTION_STRATEGY;
import static io.servicetalk.http.api.StreamingHttpConnectionToHttpConnection.DEFAULT_ASYNC_CONNECTION_STRATEGY;
import static java.util.Objects.requireNonNull;

final class StreamingHttpClientToHttpClient implements HttpClient {
Expand All @@ -32,7 +32,7 @@ final class StreamingHttpClientToHttpClient implements HttpClient {
private final HttpRequestResponseFactory reqRespFactory;

StreamingHttpClientToHttpClient(final StreamingHttpClient client, final HttpExecutionStrategy strategy) {
this.strategy = defaultStrategy() == strategy ? DEFAULT_CONNECTION_STRATEGY : strategy;
this.strategy = defaultStrategy() == strategy ? DEFAULT_ASYNC_CONNECTION_STRATEGY : strategy;
this.client = client;
context = new DelegatingHttpExecutionContext(client.executionContext()) {
@Override
Expand Down Expand Up @@ -118,7 +118,7 @@ static final class ReservedStreamingHttpConnectionToReservedHttpConnection imple

ReservedStreamingHttpConnectionToReservedHttpConnection(final ReservedStreamingHttpConnection connection,
final HttpExecutionStrategy strategy) {
this(connection, defaultStrategy() == strategy ? DEFAULT_CONNECTION_STRATEGY : strategy,
this(connection, defaultStrategy() == strategy ? DEFAULT_ASYNC_CONNECTION_STRATEGY : strategy,
toAggregated(connection));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ final class StreamingHttpConnectionToHttpConnection implements HttpConnection {
* For aggregation, we invoke the user callback (Single from client#request()) after the payload is completed,
* hence we need to offload data.
*/
static final HttpExecutionStrategy DEFAULT_CONNECTION_STRATEGY = OFFLOAD_RECEIVE_DATA_EVENT_STRATEGY;
static final HttpExecutionStrategy DEFAULT_ASYNC_CONNECTION_STRATEGY = OFFLOAD_RECEIVE_DATA_EVENT_STRATEGY;
private final StreamingHttpConnection connection;
private final HttpExecutionStrategy strategy;
private final HttpConnectionContext context;
Expand All @@ -37,7 +37,7 @@ final class StreamingHttpConnectionToHttpConnection implements HttpConnection {

StreamingHttpConnectionToHttpConnection(final StreamingHttpConnection connection,
final HttpExecutionStrategy strategy) {
this.strategy = defaultStrategy() == strategy ? DEFAULT_CONNECTION_STRATEGY : strategy;
this.strategy = defaultStrategy() == strategy ? DEFAULT_ASYNC_CONNECTION_STRATEGY : strategy;
this.connection = connection;
final HttpConnectionContext originalCtx = connection.connectionContext();
executionContext = new DelegatingHttpExecutionContext(connection.executionContext()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ void add(StreamingHttpConnectionFilterFactory connectionFilter) {
}
}

HttpExecutionStrategy buildForClient(HttpExecutionStrategy transportStrategy) {
HttpExecutionStrategy buildForClient(HttpExecutionStrategy builderStrategy) {
HttpExecutionStrategy chainStrategy = clientChain;
if (null != connFilterChain) {
chainStrategy = null != chainStrategy ? chainStrategy.merge(connFilterChain) : connFilterChain;
Expand All @@ -138,9 +138,10 @@ HttpExecutionStrategy buildForClient(HttpExecutionStrategy transportStrategy) {
}

return (null == chainStrategy || !chainStrategy.hasOffloads()) ?
transportStrategy :
defaultStrategy() == transportStrategy || !transportStrategy.hasOffloads() ?
chainStrategy : chainStrategy.merge(transportStrategy);
builderStrategy :
defaultStrategy() == builderStrategy ?
chainStrategy : builderStrategy.hasOffloads() ?
chainStrategy.merge(builderStrategy) : builderStrategy;
}

ExecutionStrategy buildForConnectionFactory() {
Expand Down
Loading