Feature/framing #575
Merged
Feature/framing #575
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robertroeser
requested review from
mostroverkhov,
nebhale and
rdegnan
and removed request for
nebhale
* optimize request channel to let you peak at the first item in the requestChannel Signed-off-by: Robert Roeser <rroeserr@gmail.com> * updated javadoc Signed-off-by: Robert Roeser <rroeserr@gmail.com> * updates Signed-off-by: Robert Roeser <rroeserr@gmail.com> * call requestChannelWith 2 arguments Signed-off-by: Robert Roeser <rroeserr@gmail.com> Signed-off-by: Robert Roeser <rroeserr@gmail.com>
Signed-off-by: Robert Roeser <rroeserr@gmail.com>
Signed-off-by: Robert Roeser <rroeserr@gmail.com>
Signed-off-by: Robert Roeser <rroeserr@gmail.com>
Signed-off-by: Robert Roeser <rroeserr@gmail.com>
… PayloadDecoder Signed-off-by: Robert Roeser <rroeserr@gmail.com>
robertroeser
force-pushed
the
feature/framing
branch
from
15f2e6e to
533d597
Compare
Signed-off-by: Robert Roeser <rroeserr@gmail.com>
Signed-off-by: Robert Roeser <rroeserr@gmail.com>
Signed-off-by: Robert Roeser <rroeserr@gmail.com>
Signed-off-by: Robert Roeser <rroeserr@gmail.com>
… PayloadDecoder Signed-off-by: Robert Roeser <rroeserr@gmail.com>
…o feature/framing
yschimke
reviewed
Feb 9, 2019
| return new DefaultConnectionSetupPayload(setupFrame); | ||
| } | ||
|
|
||
Member
There was a problem hiding this comment.
Are we having some format war? Every PR recently seems to reformat more lines than it changes...
Member
Author
There was a problem hiding this comment.
I didn't run a formatter on purpose I was going to wait till after this pull request and then make the formatter required for a build to pass.
yschimke
reviewed
Feb 9, 2019
| * @throws NullPointerException if {@code byteBufAllocator} or {@code delegate} are {@code null} | ||
| * @throws IllegalArgumentException if {@code maxFragmentSize} is not {@code positive} | ||
| */ | ||
| *//* |
Member
There was a problem hiding this comment.
Do you want to check in this commented code?
Member
Author
There was a problem hiding this comment.
yeah - should have added to my pull request that its not quite done yet, and there's a couple things left. I wanted people to start looking at it.
Member
There was a problem hiding this comment.
@yschimke just removed one commented block (not this one though), so those will be cleaned out also - one change at a time.
yschimke
reviewed
Feb 9, 2019
|
|
||
| final class LocalTransportTest implements TransportTest { | ||
|
|
||
| final class LocalTransportTest {// implements TransportTest { |
Member
There was a problem hiding this comment.
Is this WIP or you want to land, if the latter can you link to a github issue?
Member
|
Overall the change looks sensible, but I'm probably not the best to give the final shipit. Awkward with review with the large amount of reformatted and also large commented out blocks. |
mostroverkhov
suggested changes
Feb 10, 2019
rsocket-core/src/main/java/io/rsocket/frame/SetupFrameFlyweight.java
Outdated
Show resolved
Hide resolved
rsocket-core/src/main/java/io/rsocket/frame/SetupFrameFlyweight.java
Outdated
Show resolved
Hide resolved
rsocket-core/src/main/java/io/rsocket/frame/RequestStreamFrameFlyweight.java
Outdated
Show resolved
Hide resolved
rsocket-core/src/main/java/io/rsocket/frame/RequestFlyweight.java
Outdated
Show resolved
Hide resolved
rsocket-core/src/main/java/io/rsocket/frame/RequestFireAndForgetFrameFlyweight.java
Outdated
Show resolved
Hide resolved
rsocket-core/src/main/java/io/rsocket/frame/LeaseFlyweight.java
Outdated
Show resolved
Hide resolved
rsocket-core/src/main/java/io/rsocket/frame/LeaseFlyweight.java
Outdated
Show resolved
Hide resolved
rsocket-core/src/main/java/io/rsocket/frame/ExtensionFrameFlyweight.java
Outdated
Show resolved
Hide resolved
rsocket-core/src/main/java/io/rsocket/frame/FrameHeaderFlyweight.java
Outdated
Show resolved
Hide resolved
restore micrometer RSocket test Signed-off-by: Maksym Ostroverkhov <m.ostroverkhov@gmail.com>
mostroverkhov
force-pushed
the
feature/framing
branch
from
46e4f5c to
d74e7eb
Compare
release ByteBufs in tests Signed-off-by: Maksym Ostroverkhov <m.ostroverkhov@gmail.com>
mostroverkhov
force-pushed
the
feature/framing
branch
from
2ff24a1 to
b071705
Compare
nebhale
approved these changes
Feb 11, 2019
Member
nebhale
left a comment
nebhale
left a comment
There was a problem hiding this comment.
Huge improvement in consistency and readability.
rdegnan
approved these changes
Feb 11, 2019
Member
Author
|
@mostroverkhov - you good with the changes? Can I merge this? |
mostroverkhov
approved these changes
Feb 12, 2019
Member
|
@robertroeser There is still missing support for fragmentation, but that can be done is follow-up PR |
mostroverkhov
pushed a commit
to mostroverkhov/rsocket-java
that referenced
this pull request
Feb 27, 2019
* Adds specific requestChannel(Payload, Publisher<Payload>) (rsocket#572) * optimize request channel to let you peak at the first item in the requestChannel Signed-off-by: Robert Roeser <rroeserr@gmail.com> * updated javadoc Signed-off-by: Robert Roeser <rroeserr@gmail.com> * updates Signed-off-by: Robert Roeser <rroeserr@gmail.com> * call requestChannelWith 2 arguments Signed-off-by: Robert Roeser <rroeserr@gmail.com> Signed-off-by: Robert Roeser <rroeserr@gmail.com> * first pass at refactoring to idomatic ByteBuf flyweights Signed-off-by: Robert Roeser <rroeserr@gmail.com> * fixing tests Signed-off-by: Robert Roeser <rroeserr@gmail.com> * fixed some more tests Signed-off-by: Robert Roeser <rroeserr@gmail.com> * fix frame flyweights Signed-off-by: Robert Roeser <rroeserr@gmail.com> * testing Signed-off-by: Robert Roeser <rroeserr@gmail.com> * accept ByteBufAllocator in a constructors and renames FrameDecoder to PayloadDecoder Signed-off-by: Robert Roeser <rroeserr@gmail.com> * javadoc Signed-off-by: Robert Roeser <rroeserr@gmail.com> * first pass at refactoring to idomatic ByteBuf flyweights Signed-off-by: Robert Roeser <rroeserr@gmail.com> * fixing tests Signed-off-by: Robert Roeser <rroeserr@gmail.com> * fixed some more tests Signed-off-by: Robert Roeser <rroeserr@gmail.com> * testing Signed-off-by: Robert Roeser <rroeserr@gmail.com> * fix frame flyweights Signed-off-by: Robert Roeser <rroeserr@gmail.com> * accept ByteBufAllocator in a constructors and renames FrameDecoder to PayloadDecoder Signed-off-by: Robert Roeser <rroeserr@gmail.com> * javadoc Signed-off-by: Robert Roeser <rroeserr@gmail.com> * setup frame flyweight: allow null metadata restore micrometer RSocket test Signed-off-by: Maksym Ostroverkhov <m.ostroverkhov@gmail.com> * apply fixes from review release ByteBufs in tests Signed-off-by: Maksym Ostroverkhov <m.ostroverkhov@gmail.com> Signed-off-by: Maksym Ostroverkhov <m.ostroverkhov@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Originally RSocket didn't use ByteBuf, and the existing frame classes are a legacy of that. This change finishes the framing work @nebhale started, and turns all the Frames into flyweights. It removes the Frame class completely. The frames originally required offsets and this has been replaced using ByteBuf's reader and writer indexes.
Frame length is not long part of the header flyweight. It is a separate flyweight that can be used by transports that require a length. It only needs to be applied to in the duplex connection before sending the frame.
The Payload classes remained unchanged.
Only two things are now recycled - the Payloads, and ByteBufs.
There is an API change in the RSocketFactory. The default remains unchanged but instead of taking a function that turns a ByteBuf into a Payload it now takes a specific type called a PayloadDecoder. There are constants in the interface to a zero copy implementation and a default copying implementation. There is also a method added to let the call specify a ByteBufAllocator if they want to change from ByteBufAllocator.DEFAULT
These changes - aside from RSocketFactory - are transparent.