-
Notifications
You must be signed in to change notification settings - Fork 910
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
Add PathStreamMessage
that publishes the contents of a Path
#3344
Conversation
Motivation: It must be nice to create `StreamMessage` from a `File` or `Path`. Modifications: - Add `PathStreamMessage` that reads bytes from a `Path` and publishes with backpressure - Add useful factories to `StreamMessage` Result: You can now create a `StreamMessage` from a `Path` or `File`. ```java Path path = Paths.get("..."); StreamMessage<HttpData> publisher = StreamMessage.of(path); ```
PathStreamMessage
that publishes a Path
PathStreamMessage
that publishes the contents of a Path
Codecov Report
@@ Coverage Diff @@
## master #3344 +/- ##
============================================
+ Coverage 74.19% 74.26% +0.06%
- Complexity 13293 13324 +31
============================================
Files 1159 1160 +1
Lines 50740 50918 +178
Branches 6504 6530 +26
============================================
+ Hits 37646 37812 +166
- Misses 9775 9782 +7
- Partials 3319 3324 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks! 😄
core/src/main/java/com/linecorp/armeria/common/stream/PathStreamMessage.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/PathStreamMessage.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/PathStreamMessage.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/StreamMessage.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, @ikhoon!
core/src/main/java/com/linecorp/armeria/common/stream/StreamMessage.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/PathStreamMessage.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/PathStreamMessage.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/PathStreamMessage.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/PathStreamMessage.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/PathStreamMessage.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/PathStreamMessage.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/StreamMessage.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/common/stream/PathStreamMessageTest.java
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,368 @@ | |||
/* | |||
* Copyright 2020 LINE Corporation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2021
|
||
private static final Logger logger = LoggerFactory.getLogger(PathStreamMessage.class); | ||
|
||
static final int DEFAULT_FILE_BUFFER_SIZE = 4096; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe 8192? 4K may be too small for modern OSes.
byteBuf.release(); | ||
maybeCloseFileChannel(); | ||
} else { | ||
if (result > -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>= 0
?
byteBuf.writerIndex(result); | ||
final HttpData data; | ||
if (withPooledObjects) { | ||
data = HttpData.wrap(byteBuf); | ||
} else { | ||
data = HttpData.wrap(ByteBufUtil.getBytes(byteBuf)); | ||
byteBuf.release(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do this without updating reader/writerIndex
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, we need to call byteBuf.writerIndex(result)
because the indexes of byteBuf
are not changed by ByteBuffer
.
https://github.com/netty/netty/blob/e5951d46fc89db507ba7d2968d2ede26378f0b04/buffer/src/main/java/io/netty/buffer/ByteBuf.java#L2292-L2294
* Exposes this buffer's sub-region as an NIO {@link ByteBuffer}. The returned buffer * either share or contains the copied content of this buffer, while changing the position * and limit of the returned NIO buffer does not affect the indexes and marks of this buffer.
} catch (IOException ignored) { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe log at WARN?
@ArgumentsSource(SubscriptionOptionsProvider.class) | ||
@ParameterizedTest | ||
void readFile(SubscriptionOption[] options) { | ||
final Path path = Paths.get("src/test/resources/com/linecorp/armeria/common/stream/test.txt"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we test a file larger than the buffer size as well as a small one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job, @ikhoon !
Motivation:
It must be nice to create
StreamMessage
from aFile
orPath
.Modifications:
PathStreamMessage
that reads bytes from aPath
and publishes with backpressureStreamMessage
Result:
You can now create a
StreamMessage
from aPath
orFile
.