FileRegion support in HTTP/2 #3928
Conversation
|
AUTOMATED MESSAGE for ADMINS: |
| /** | ||
| * Wrap a DATA frame so it can be written subject to flow-control. Note that this implementation assumes it | ||
| * only writes padding once for the entire payload as opposed to writing it once per-frame. This makes the | ||
| * {@link #size} calculation deterministic thereby greatly simplifying the implementation. |
There was a problem hiding this comment.
{@link #size} -> {@link #size()}
|
@netkins accept |
|
@Apache9 - I took a first pass. Looks good so far! Ping me with questions and when ready for another round of review. |
|
@Scottmitch Great! Let me address the issues you mentioned. Thanks. |
|
@Scottmitch I have updated the patch. Thanks. |
| @@ -400,10 +417,10 @@ public void operationComplete(ChannelFuture future) throws Exception { | |||
|
|
|||
| @Override | |||
| public boolean merge(Http2RemoteFlowController.FlowControlled next) { | |||
| if (FlowControlledData.class != next.getClass()) { | |||
| if (FlowControlledByteBuf.class != next.getClass()) { | |||
There was a problem hiding this comment.
just getClass() != next.getClass()?
|
@Apache9 took a first pass ... just a few comments. Looks pretty good overall! |
| @Override | ||
| public void write(int allowedBytes) { | ||
| int bytesWritten = 0; | ||
| if (data == null || (allowedBytes == 0 && size != 0)) { |
There was a problem hiding this comment.
I agree with @nmittler's above comment. Either try to unify the 2 algorithms or keep them separate.
It may be good to unify these two methods to make sure both cases work. For example in this case you are setting data = null to signify you are done writing data, and may have padding left...but here you are checking data == null and exiting before checking if any padding needs to be written. The ByteBuf accounts for this by only setting data = null on release, and EMPTY_BUF if the write process exhausted the available data in the buffer.
There was a problem hiding this comment.
o, it is ||, not &&. Let me fix it.
I separated the logic of writing padding only and writing frame because we do not have an EmptyFileRegion and it makes no sense to have an EmptyFileRegion I think? There must be a file associate with a FileRegion.
|
@Apache9 - Few more comments. Lets also add a unit test where the size is bigger than |
Yes, let me try. |
Add FileRegion support in HTTP/2 Modifications: Add a writeData method in Http2DataWriter which accept FileRegion as data. Add a readSlice method in FileRegion which is like the readSlice method in ByteBuf. Change pendingBytes in DefaultHttp2RemoteFlowController and the return value of FlowControlled.size() to long. Add two testcases to test the functionality of FileRegion support in HTTP/2. Result: Now one can write a FileRegion as DATA frame.
|
@Scottmitch I have updated the patch. Rewrite DefaultFileRegion that add readSlice and unwrap. Add a Thanks. |
|
@Scottmitch @normanmaurer I wonder if there might be a way to provide a common interface between |
Motivation: Based on netty#3965. As a first step of integrating the unified reading API, the low-hanging fruit is to refactor `FIleRegion` to use it. Modifications: Refactored `FileRegion` to extend `ReadableObject` and implemented the new interface in `DefaultFileRegion`. Result: `FileRegion` implements the new `ReadableObject` interface.
POC of #3927.