Add StreamMessage.of(future) that produces StreamMessage from the future #4995
Conversation
Motivation: We have `HttpResponse.from(future)` that produces the `HttpResponse` when the future completes. It woulb be nice if we can have similar one for `HttpRequest` which produces the data and trailers when the future compeltes. Modifications: - Add `DeferredHttpRequest` which is created from `HttpRequest.from(future)`. Result: - You can now create an `HttpRequest` that produces data and trailers after a future completes.
minwoox
changed the title
minwoox
changed the title
.github/workflows/actions_build.yml
Outdated
| @@ -24,7 +24,7 @@ jobs: | |||
| build: | |||
| if: github.repository == 'line/armeria' | |||
| runs-on: ${{ matrix.on }} | |||
| timeout-minutes: 100 | |||
| timeout-minutes: 120 | |||
There was a problem hiding this comment.
Is this change intentional?
There was a problem hiding this comment.
We should revert this before merging.
| * @param future the {@link CompletableFuture} which will produce the actual data and trailers | ||
| */ | ||
| static HttpRequest from(RequestHeaders headers, | ||
| CompletableFuture<? extends StreamMessage<? extends HttpObject>> future) { |
There was a problem hiding this comment.
Question) Is this API needed since CompletionFuture inherits CompletionStage?
There was a problem hiding this comment.
Oops, will remove it.
| * | ||
| * @param stage the {@link CompletionStage} which will produce the actual data and trailers | ||
| */ | ||
| static HttpRequest from(RequestHeaders headers, |
There was a problem hiding this comment.
Question) would it rather make more sense to name this method of, and create the counterparts in HttpResponse? (and deprecate from() factory methods)
There was a problem hiding this comment.
Changed to of. 😉
For HttpResponse, I actually left TODO for it because it needs to change quite a lot.
Let me send another PR for that. 😉
| * the returned {@link StreamMessage} using {@link #subscribe(Subscriber)} | ||
| * or {@link #subscribe(Subscriber, SubscriptionOption...)}. | ||
| */ | ||
| static <T> StreamMessage<T> of(CompletionStage<? extends StreamMessage<? extends T>> stage, |
There was a problem hiding this comment.
I think optionally we could also apply the CF completion check here too (I find it odd that only some APIs do this optimization, while others don't)
There was a problem hiding this comment.
If so, we cannot use the executor specified. 😉 Let me add a comment there.
core/src/main/java/com/linecorp/armeria/common/stream/StreamMessage.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/StreamMessage.java
Outdated
Show resolved
Hide resolved
| * the returned {@link StreamMessage} using {@link #subscribe(Subscriber)} | ||
| * or {@link #subscribe(Subscriber, SubscriptionOption...)}. | ||
| */ | ||
| static <T> StreamMessage<T> of(CompletionStage<? extends StreamMessage<? extends T>> stage, |
There was a problem hiding this comment.
What do you think of adding of(CompletableFuture<StreamMesage<T>> future, EventExecutor) for consistency, otherwise, removing of(CompletableFuture<StreamMesage<T>> future) from public API?
There was a problem hiding this comment.
removing of(CompletableFuture<StreamMesage> future) from public API?
+1 for this. I will also deprecate/remove HttpResponse.from(CompletableFuture<? extends HttpResponse> future). 😉
|
@ikhoon Forgot to address the comments. 😓 All addressed. PTAL. 😉 |
|
Thanks for reviewing. 😉 |
Motivation: We have decided to deprecate `HttpResponse.from(...)` methods and use `HttpResponse.of(...)` instead for consistency. line#4995 (comment) Modification: - Deprecate `HttpResponse.from(CompletionStage)` and its variants. - `HttpResponse.of(CompletionStage)` and its variants are added. Result: - `HttpResponse.from(CompletionStage)` and its variants methods are deprecated. - Use `HttpResponse.of(CompletionStage)` and its variants instead.
Motivation: We have decided to deprecate `HttpResponse.from(...)` methods and use `HttpResponse.of(...)` instead for consistency. #4995 (comment) Modification: - Deprecate `HttpResponse.from(CompletionStage)` and its variants. - `HttpResponse.of(CompletionStage)` and its variants are added. Result: - `HttpResponse.from(CompletionStage)` and its variants methods are deprecated. - Use `HttpResponse.of(CompletionStage)` and its variants instead.
Motivation:
We have
HttpResponse.from(future)that produces theHttpResponsewhen the future completes. It would be nice if we can have it onStreamMessagelevel.Modifications:
StreamMessage.of(future)that produces another StreamMessage from the futureResult:
StreamMessageusing a future.