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

Remove HttpExecutionStrategy from requester and reserveConnection API #1956

Merged
merged 5 commits into from
Nov 19, 2021

Conversation

idelpivnitskiy
Copy link
Member

Motivation:

#1904 allows a request to have a context associated with it. It can be
used to propagate additional meta-information to the transport layers,
like a required HttpExecutionStrategy or FlushStrategy. It allows us
to simplify requester and filters API and remove an overload that
requires HttpExecutionStrategy from there.

Modifications:

  • Add HttpContextKeys with HTTP_EXECUTION_STRATEGY_KEY;
  • Promote request(RequestType) method to StreamingHttpRequester,
    HttpRequester, BlockingHttpRequester, and
    BlockingStreamingHttpRequester and deprecate
    request(HttpExecutionStrategy, RequestType) method;
  • Deprecate
    reserveConnection(HttpExecutionStrategy, HttpRequestMetaData) method
    in FilterableStreamingHttpClient, HttpClient, BlockingHttpClient,
    BlockingStreamingHttpClient;
  • All default implementation delegate to a new method;
  • Add NewToDeprecatedFilter to allow users to have mixed filters in
    the pipeline (migrated and deprecated);
  • MixedFiltersTest: test all different combinations of filters to make
    sure both can work together;
  • Remove unused FilterableConnectionToConnection;
  • Update evolve-to-async.adoc;

Result:

Less overloads on client and requester API, simplified filtering,
users can incrementally migrate to simplified API.

Verified

This commit was signed with the committer’s verified signature.
reta Andriy Redko
Motivation:

apple#1904 allows a request to have a context associated with it. It can be
used to propagate additional meta-information to the transport layers,
like a required `HttpExecutionStrategy` or `FlushStrategy`. It allows us
to simplify requester and filters API and remove an overload that
requires `HttpExecutionStrategy` from there.

Modifications:

- Add `HttpContextKeys` with `HTTP_EXECUTION_STRATEGY_KEY`;
- Promote `request(RequestType)` method to `StreamingHttpRequester`,
`HttpRequester`, `BlockingHttpRequester`, and
`BlockingStreamingHttpRequester` and deprecate
`request(HttpExecutionStrategy, RequestType)` method;
- Deprecate
`reserveConnection(HttpExecutionStrategy, HttpRequestMetaData)` method
in `FilterableStreamingHttpClient`, `HttpClient`, `BlockingHttpClient`,
`BlockingStreamingHttpClient`;
- All default implementation delegate to a new method;
- Add `NewToDeprecatedFilter` to allow users to have mixed filters in
the pipeline (migrated and deprecated);
- `MixedFiltersTest`: test all different combinations of filters to make
sure both can work together;
- Remove unused `FilterableConnectionToConnection`;
- Update `evolve-to-async.adoc`;

Result:

Less overloads on client and requester API, simplified filtering,
users can incrementally migrate to simplified API.
idelpivnitskiy added a commit to idelpivnitskiy/servicetalk that referenced this pull request Nov 18, 2021
Motivation:

apple#1956 removed requester/client methods that take
`HttpExecutionStrategy`. All deprecated API can be removed now.

Modifications:

- Remove deprecated `request` method from a requester;
- Remove deprecated `reserveConnection` method from a client;
- Remove `NewToDeprecated` utility;
- Migrate all existing filters and tests to the new filter API;
- Pass an `HttpExecutionStrategy` through request context;

Result:

No deprecated API on requester/client/filter API.
@idelpivnitskiy
Copy link
Member Author

idelpivnitskiy commented Nov 18, 2021

59dac1c from #1960 demonstrates what users will have to do to migrate their filters.

@idelpivnitskiy
Copy link
Member Author

I will merge this PR to finish cleanup. If someone has more comments, will address in a follow-up

@idelpivnitskiy idelpivnitskiy merged commit 1ba30f1 into apple:main Nov 19, 2021
@idelpivnitskiy idelpivnitskiy deleted the filter-api branch November 19, 2021 16:48
idelpivnitskiy added a commit that referenced this pull request Nov 19, 2021
…PI (#1956)

Motivation:

used to propagate additional meta-information to the transport layers,
like a required `HttpExecutionStrategy` or `FlushStrategy`. It allows us
to simplify requester and filters API and remove an overload that
requires `HttpExecutionStrategy` from there.

Modifications:

- Add `HttpContextKeys` with `HTTP_EXECUTION_STRATEGY_KEY`;
- Promote `request(RequestType)` method to `StreamingHttpRequester`,
`HttpRequester`, `BlockingHttpRequester`, and
`BlockingStreamingHttpRequester` and deprecate
`request(HttpExecutionStrategy, RequestType)` method;
- Deprecate
`reserveConnection(HttpExecutionStrategy, HttpRequestMetaData)` method
in `FilterableStreamingHttpClient`, `HttpClient`, `BlockingHttpClient`,
`BlockingStreamingHttpClient`;
- All default implementation delegate to a new method;
- Add `NewToDeprecatedFilter` to allow users to have mixed filters in
the pipeline (migrated and deprecated);
- `MixedFiltersTest`: test all different combinations of filters to make
sure both can work together;
- Remove unused `FilterableConnectionToConnection`;
- Update `evolve-to-async.adoc`;

Result:

Less overloads on client and requester API, simplified filtering,
users can incrementally migrate to simplified API.
idelpivnitskiy added a commit that referenced this pull request Nov 19, 2021
…PI (#1956)

Motivation:

used to propagate additional meta-information to the transport layers,
like a required `HttpExecutionStrategy` or `FlushStrategy`. It allows us
to simplify requester and filters API and remove an overload that
requires `HttpExecutionStrategy` from there.

Modifications:

- Add `HttpContextKeys` with `HTTP_EXECUTION_STRATEGY_KEY`;
- Promote `request(RequestType)` method to `StreamingHttpRequester`,
`HttpRequester`, `BlockingHttpRequester`, and
`BlockingStreamingHttpRequester` and deprecate
`request(HttpExecutionStrategy, RequestType)` method;
- Deprecate
`reserveConnection(HttpExecutionStrategy, HttpRequestMetaData)` method
in `FilterableStreamingHttpClient`, `HttpClient`, `BlockingHttpClient`,
`BlockingStreamingHttpClient`;
- All default implementation delegate to a new method;
- Add `NewToDeprecatedFilter` to allow users to have mixed filters in
the pipeline (migrated and deprecated);
- `MixedFiltersTest`: test all different combinations of filters to make
sure both can work together;
- Remove unused `FilterableConnectionToConnection`;
- Update `evolve-to-async.adoc`;

Result:

Less overloads on client and requester API, simplified filtering,
users can incrementally migrate to simplified API.
bondolo pushed a commit to bondolo/servicetalk that referenced this pull request Nov 19, 2021
…PI (apple#1956)

Motivation:

apple#1904 allows a request to have a context associated with it. It can be
used to propagate additional meta-information to the transport layers,
like a required `HttpExecutionStrategy` or `FlushStrategy`. It allows us
to simplify requester and filters API and remove an overload that
requires `HttpExecutionStrategy` from there.

Modifications:

- Add `HttpContextKeys` with `HTTP_EXECUTION_STRATEGY_KEY`;
- Promote `request(RequestType)` method to `StreamingHttpRequester`,
`HttpRequester`, `BlockingHttpRequester`, and
`BlockingStreamingHttpRequester` and deprecate
`request(HttpExecutionStrategy, RequestType)` method;
- Deprecate
`reserveConnection(HttpExecutionStrategy, HttpRequestMetaData)` method
in `FilterableStreamingHttpClient`, `HttpClient`, `BlockingHttpClient`,
`BlockingStreamingHttpClient`;
- All default implementation delegate to a new method;
- Add `NewToDeprecatedFilter` to allow users to have mixed filters in
the pipeline (migrated and deprecated);
- `MixedFiltersTest`: test all different combinations of filters to make
sure both can work together;
- Remove unused `FilterableConnectionToConnection`;
- Update `evolve-to-async.adoc`;

Result:

Less overloads on client and requester API, simplified filtering,
users can incrementally migrate to simplified API.
idelpivnitskiy added a commit to idelpivnitskiy/servicetalk that referenced this pull request Dec 2, 2021
Motivation:

apple#1956 introduced `NEW_TO_DEPRECATED_FILTER` which helps to glue a
new filter API to deprecated filter API. This glue was missed in
`AbstractHttpRequesterFilterTest`.

Modifications:

- Add `NEW_TO_DEPRECATED_FILTER` in
`AbstractHttpRequesterFilterTest#newClient`;

Result:

Implementations of `AbstractHttpRequesterFilterTest` work correctly.
chemicL pushed a commit that referenced this pull request Dec 2, 2021
Motivation:

#1956 introduced `NEW_TO_DEPRECATED_FILTER` which helps to glue a
new filter API to deprecated filter API. This glue was missed in
`AbstractHttpRequesterFilterTest`.

Modifications:

- Add `NEW_TO_DEPRECATED_FILTER` in
`AbstractHttpRequesterFilterTest#newClient`;

Result:

Implementations of `AbstractHttpRequesterFilterTest` work correctly.
idelpivnitskiy added a commit to idelpivnitskiy/servicetalk that referenced this pull request Feb 11, 2022


Motivation:

In apple#1956 `StreamingHttpRequester` defines a new method
`request(StreamingHttpRequest)`. `AbstractStreamingHttpConnection` only
implements the deprecated method that also takes an
`HttpExecutionStrategy`. Users who apply a filter through
`ConnectionFactory` see `UnsupportedOperationException`. Also, if they
have 2+ filters applied through `ConnectionFactory`, they can not mix
deprecated and new API.

Modifications:

- Implement
`AbstractStreamingHttpConnection#request(StreamingHttpRequest)`;
- Enhance `NewToDeprecatedFilter` to be `ConnectionFactoryFilter` too;
- Apply `NewToDeprecatedFilter` when users invoke
`appendConnectionFactoryFilter`;
- Test `appendConnectionFactoryFilter` in `MixedFiltersTest`;

Result:

Users who apply request/response filters through `ConnectionFactory`
can migrate from deprecated to new `request(StreamingHttpRequest)` API.
idelpivnitskiy added a commit that referenced this pull request Feb 18, 2022
…2089)

Motivation:

In #1956 `StreamingHttpRequester` defines a new method
`request(StreamingHttpRequest)`. `AbstractStreamingHttpConnection` only
implements the deprecated method that also takes an
`HttpExecutionStrategy`. Users who apply a filter through
`ConnectionFactory` see `UnsupportedOperationException`. Also, if they
have 2+ filters applied through `ConnectionFactory`, they can not mix
deprecated and new API.

Modifications:

- Implement
`AbstractStreamingHttpConnection#request(StreamingHttpRequest)`;
- Enhance `NewToDeprecatedFilter` to be `ConnectionFactoryFilter` too;
- Apply `NewToDeprecatedFilter` when users invoke
`appendConnectionFactoryFilter`;
- Test `appendConnectionFactoryFilter` in `MixedFiltersTest`;

Result:

Users who apply request/response filters through `ConnectionFactory`
can migrate from deprecated to new `request(StreamingHttpRequest)` API.
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.

None yet

2 participants