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.

Sign up for GitHub

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

Jump to bottom

Add StreamMessage.of(inputStream) #4703

Merged
merged 20 commits into from Mar 13, 2023
Merged

Add StreamMessage.of(inputStream) #4703

merged 20 commits into from Mar 13, 2023

Conversation

injae-kim
Copy link
Contributor

Related Issue #3937

Motivation:

static StreamMessage<HttpData> of(InputStream inputStream) {...}
  • It would be nice if we provide a way to convert an InputStream to a StreamMessage that splits the binary by the bufferSize.

Modifications:

  • Add StreamMessage.of(inputStream)
  • Add InputStreamStreamMessage and builder

Result:

@minwoox minwoox added this to the 1.23.0 milestone Feb 25, 2023
@injae-kim injae-kim force-pushed the add-stream-message-input-stream branch from a68980d to 7c7de9c Compare February 25, 2023 17:10
@ikhoon
Copy link
Contributor

ikhoon commented Feb 27, 2023 •
edited

@injae-kim Why don't you send a PR to https://github.com/softwaremill/tapir in which I left a TODO for this issue when a new version is released? 😉
https://github.com/softwaremill/tapir/blob/master/server/armeria-server/src/main/scala/sttp/tapir/server/armeria/ArmeriaToResponseBody.scala#L52-L53

ikhoon
ikhoon reviewed Feb 27, 2023
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks good.

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Outdated Show resolved Hide resolved
@injae-kim
Copy link
Contributor Author

injae-kim commented Feb 27, 2023 •
edited

// TODO(ikhoon): Add StreamMessage.of(InputStream)
Why don't you send a PR to https://github.com/softwaremill/tapir in which I left a TODO for this issue when a new version is released? 😉

oh~ after this PR is merged and released with new version, I'll also send PR to tapir to remove TODO~!

trustin
trustin reviewed Feb 28, 2023
View reviewed changes
core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java
Comment on lines +135 to +144
if (this.blockingTaskExecutor != null) {
blockingTaskExecutor = this.blockingTaskExecutor;
} else {
final ServiceRequestContext serviceRequestContext = ServiceRequestContext.currentOrNull();
if (serviceRequestContext != null) {
blockingTaskExecutor = serviceRequestContext.blockingTaskExecutor();
} else {
blockingTaskExecutor = CommonPools.blockingTaskExecutor();
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see some duplications between this class and PathStreamMessage. Can we deduplicate as much as possible, potentially introducing a common supertype?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we deduplicate as much as possible, potentially introducing a common supertype?

SGTM! but may I handle it with another issue & PR?
Cause I think we need some discussion about this issue 🙇

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to handle it in a separate issue, it is okay to send a clean-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll send clean-up PR after this PR is merged~!

core/src/test/java/com/linecorp/armeria/common/stream/InputStreamStreamMessageTest.java Show resolved Hide resolved
jrhee17
jrhee17 reviewed Mar 7, 2023
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of correctness InputStreamStreamMessage looks good 👍 I think this PR is almost done! 🚀 Left only nit comments 🙇

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/AbstractByteStreamMessageBuilder.java Show resolved Hide resolved
jrhee17
jrhee17 approved these changes Mar 8, 2023
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changeset looks good to me (bar the comment #4703 (comment) which I think you'll be following up on) 😄
Thanks @injae-kim 👍 🙇 👍

core/src/main/java/com/linecorp/armeria/common/stream/StreamMessage.java Show resolved Hide resolved
minwoox
minwoox reviewed Mar 8, 2023
View reviewed changes
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Left some suggestions and a question. 😄

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessageBuilder.java Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/StreamMessage.java Outdated Show resolved Hide resolved
minwoox
minwoox approved these changes Mar 9, 2023
View reviewed changes
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for adding this nice feature! 👍 🙇 👍

core/src/main/java/com/linecorp/armeria/common/stream/PathStreamMessageBuilder.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/PathStreamMessageBuilder.java Outdated Show resolved Hide resolved
@injae-kim injae-kim force-pushed the add-stream-message-input-stream branch from 796198e to 5898efa Compare March 10, 2023 08:45
trustin
trustin approved these changes Mar 10, 2023
View reviewed changes
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 once all others comments are addressed.

ikhoon
ikhoon approved these changes Mar 13, 2023
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! 🚀💯

@ikhoon ikhoon merged commit 0d94d0d into line:master Mar 13, 2023
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrhee17 jrhee17 jrhee17 approved these changes

@trustin trustin trustin approved these changes

@ikhoon ikhoon ikhoon approved these changes

@minwoox minwoox minwoox approved these changes

Assignees
No one assigned
Labels
new feature
Projects
None yet
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.

Add StreamMessage.of(inputStream, bufferSize)
5 participants
@injae-kim @ikhoon @trustin @jrhee17 @minwoox