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.

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

improves DuplexConnection api and reworks Resumability #923

Merged
merged 6 commits into from
Sep 14, 2020

Conversation

OlegDokuka
Copy link
Member

This PR enhances DuplexConnection API to unify the way how frames should be sent over it

Signed-off-by: Oleh Dokuka shadowgun@i.ua

@OlegDokuka OlegDokuka added breaking changes Changes that break API or behaviour enhancement labels Aug 31, 2020
@OlegDokuka OlegDokuka added this to the 1.1 M2 milestone Aug 31, 2020
@OlegDokuka OlegDokuka linked an issue Aug 31, 2020 that may be closed by this pull request
@OlegDokuka OlegDokuka changed the title provides enhanced DuplexConnection api improves DuplexConnection api Aug 31, 2020
@OlegDokuka OlegDokuka force-pushed the enhancement/new-duplex-connection-design branch 5 times, most recently from 7c26049 to ac0d169 Compare September 6, 2020 17:33
@OlegDokuka
Copy link
Member Author

OlegDokuka commented Sep 6, 2020

@rstoyanchev finalized API and reworked Resumability implementation.

If API and reworked impl looks good, will be adding tests and merging

@OlegDokuka OlegDokuka force-pushed the enhancement/new-duplex-connection-design branch from ac0d169 to fd319cc Compare September 7, 2020 19:19
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

This is a partial review with some feedback identified so far.

@@ -183,21 +183,21 @@ public InternalDuplexConnection(
}

@Override
public Mono<Void> send(Publisher<ByteBuf> frame) {
public void sendFrame(int streamId, ByteBuf frame) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this duplicate copy of ClientServerInputMultplexer. The class is for internal use and this copy is no longer used. I don't see much sense in a deprecation.

In addition, the ClientServerInputMultiplexerTest is still in the internal package and still references the old class which means the new one doesn't have coverage.

return serverConnection;
}

public DuplexConnection asClientConnection() {
DuplexConnection asRequesterConnection() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Really minor but the terminology requester and responder doesn't align with the rest of this class which is client and server, including the class name. DuplexConnectionInterceptor.Type also uses client and server, and so does the spec when describing client and server stream id's. I believe that is what they are, and this is why they flip between RSocketConnector and RSocketServer where one creates the requester asRequesterConnection() while the other with asResponderConnection().

I think asClientConnection() and asServerConnection are correct.

@OlegDokuka OlegDokuka force-pushed the enhancement/new-duplex-connection-design branch from fa944a6 to 3fe88bf Compare September 13, 2020 20:55
OlegDokuka and others added 3 commits September 14, 2020 13:31
Signed-off-by: Oleh Dokuka <shadowgun@i.ua>
Signed-off-by: Oleh Dokuka <shadowgun@i.ua>
Co-authored-by: Rossen Stoyanchev <rstoyanchev@users.noreply.github.com>
@OlegDokuka OlegDokuka force-pushed the enhancement/new-duplex-connection-design branch from 3fe88bf to ba7a04f Compare September 14, 2020 11:57
Signed-off-by: Oleh Dokuka <shadowgun@i.ua>
@OlegDokuka OlegDokuka force-pushed the enhancement/new-duplex-connection-design branch from ba7a04f to 8396d4d Compare September 14, 2020 12:18
Signed-off-by: Oleh Dokuka <shadowgun@i.ua>
@OlegDokuka OlegDokuka force-pushed the enhancement/new-duplex-connection-design branch from f502089 to 545cc91 Compare September 14, 2020 12:43
Signed-off-by: Oleh Dokuka <shadowgun@i.ua>
@OlegDokuka OlegDokuka force-pushed the enhancement/new-duplex-connection-design branch from 545cc91 to e355d53 Compare September 14, 2020 12:52
@OlegDokuka OlegDokuka changed the title improves DuplexConnection api improves DuplexConnection api and reworks Resumability Sep 14, 2020
@OlegDokuka OlegDokuka merged commit 78a747a into master Sep 14, 2020
@OlegDokuka OlegDokuka deleted the enhancement/new-duplex-connection-design branch September 14, 2020 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes Changes that break API or behaviour enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve DuplexConnection design
2 participants