Proposal for TCP server at all layers (core, transport & rxjava1) #19

NiteshKant · 2015-03-20T23:38:24Z

Based on various discussions around different issues, I am proposing a fresh design for a TCP server in this PR.
I have implemented a TCP server at all layers in reactive-ipc, i.e. core, transport(netty) and rxjava1. I have also removed all code which isn't related to the implementation. We can discuss those utilities in isolation as and when required to be added. Although, not used, I have kept the Buffer and related classes as that aspect is not yet discussed to conclusion.

There are two samples each for netty transport and rxjava1 layer available to see how will a typical interaction look like in modules ripc-transport-netty4-examples and ripc-rxjava1-examples

Backpressure has not been implemented yet.

benjchristensen · 2015-03-23T17:07:31Z

@NiteshKant Can you rebase this for consideration to merge?

benjchristensen · 2015-03-23T17:08:48Z

@jbrisbin @smaldini This feels like a good first step towards getting a protocol working end-to-end with all the layers (transport, core, adaptor) and example code.

There is a lot to discuss and this is far from production worth, and the APIs are most definitely not all correct, but I propose we merge this and use it to start iterating with further pull requests and discussion.

benjchristensen · 2015-03-23T17:09:29Z

BTW, once we have a good approach we can migrate the tcp protocol code to its own project as per #18

NiteshKant · 2015-03-23T17:17:20Z

@benjchristensen the code seems to be on the latest from upstream and is ready to be merged.

jbrisbin · 2015-03-23T18:08:13Z

👍

benjchristensen · 2015-03-23T19:34:11Z

@NiteshKant The question about rebasing was whether we should retain the 7 commits for this, or squash to 1 commit.

jbrisbin · 2015-03-23T19:48:23Z

Ideally they should be squashed to 1.

Based on various discussions around different issues, I am proposing a fresh design for a TCP server in this PR. I have implemented a TCP server at all layers in reactive-ipc, i.e. core, transport(netty) and rxjava1. I have also removed all code which isn't related to the implementation. We can discuss those utilities in isolation as and when required to be added. Although, not used, I have kept the Buffer and related classes as that aspect is not yet discussed to conclusion. There are two samples each for netty transport and rxjava1 layer available to see how will a typical interaction look like in modules ripc-transport-netty4-examples and ripc-rxjava1-examples Backpressure has not been implemented yet.

NiteshKant · 2015-03-23T21:07:09Z

Looks like my attempt to squash across merge commits rendered this PR unusable :(
Lemme try to fix it

…-jvm Conflicts: ripc-protocol-tcp/src/main/java/io/ripc/protocol/tcp/Connection.java ripc-transport-netty4/src/main/java/io/ripc/transport/netty4/tcp/ChannelInboundHandlerSubscription.java ripc-transport-netty4/src/main/java/io/ripc/transport/netty4/tcp/server/NettyTcpServer.java ripc-transport-netty4/src/main/java/io/ripc/transport/netty4/tcp/server/NettyTcpServerConnection.java ripc-transport-netty4/src/test/java/io/ripc/transport/netty4/tcp/server/NettyTcpServerIntegrationTests.java

benjchristensen · 2015-03-23T22:23:32Z

You shouldn't need the merge commit. You can just rebase on top of the master branch. Or just checkout master again from scratch, branch, then copy/paste your code and commit a single clean commit.

jbrisbin · 2015-03-25T13:21:44Z

ripc-protocol-tcp/src/main/java/io/ripc/protocol/tcp/TcpHandler.java

+
+import org.reactivestreams.Publisher;
+
+public interface TcpHandler<R, W> {


As a general rule, I'm not a fan of duplicating names in both packages and class names. It also doesn't "handle" Tcp, it handles a Connection. If needing a qualifier to Handler, it should probably be ConnectionHandler.

The reason I added Tcp prefix was that Handler by itself is confusing when someone starts creating two servers with different transports in the same code. In that scenario they have to use the fully qualified name for one of the handlers. Even ConnectionHandler is ambiguous when we have a WebSocket/HTTP/2 connection handler.

The reason I added Tcp prefix was that Handler by itself is confusing when someone starts creating two servers with different transports in the same code.

Good point.

Maybe this should be applied across-the-board so that TcpConnection doesn't interfere with WebSocketConnection et al?

Maybe this should be applied across-the-board so that TcpConnection doesn't interfere with WebSocketConnection et al?

Indeed, it will be.

jbrisbin · 2015-03-25T14:18:19Z

FWIW @rstoyanchev @smaldini and I are going to be going over this in a more comprehensive way today as we individually evaluate some of these approaches and then get together to discuss them to offer more comprehensive feedback than is possible in line comments.

jbrisbin · 2015-03-25T14:34:03Z

ripc-core/src/main/java/io/ripc/internal/Publishers.java

+                s.onSubscribe(new Subscription() {
+                    @Override
+                    public void request(long n) {
+                        s.onNext(value);


There should be a positive demand check here per the spec. The Specifications class was intended to be the helper that aggregated these checks. This will be an ongoing issue (proper adherence to the spec) any time we implement helpers in the core.

Sure, just to be clear, the implementation is not feature complete. It was a strawman to start discussion in light of all layers being implemented.

The Specifications class was intended to be the helper that aggregated these checks.

Not sure if having an independent Specifications class helps in this case. How does it makes sure that everyone abides by the spec?

jbrisbin · 2015-03-25T15:14:26Z

How would we handle connection close events and reconnects?

NiteshKant · 2015-03-25T17:08:41Z

FWIW @rstoyanchev @smaldini and I are going to be going over this in a more comprehensive way today as we individually evaluate some of these approaches and then get together to discuss them to offer more comprehensive feedback than is possible in line comments.

All great questions @jbrisbin . I will reply to them so that you guys have more context on these when you discuss them.

jbrisbin · 2015-03-26T13:00:51Z

FWIW We talked about this for 3 hours yesterday and have several concrete thoughts about some of these issues. I'm putting together some code to illustrate our thoughts and I'll try and create a summary of our discussion to point out the things we think are important. I'm just trying to figure out how the concepts and suggestions encoded in this PR converge with some of the other things we talked about and how we could bring them closer together.

TL;DR we don't think this PR is all that far away from where we probably need to be to do the kinds of things we need to do in the varied use cases we have (many of which vary quite radically from one another). That said, we do find a few things problematic and I'll try and explain why and demonstrate with code in a separate PR, which we can hopefully use as discussion points to iterate again on something that meets both needs.

NiteshKant · 2015-03-26T16:30:43Z

we don't think this PR is all that far away from where we probably need to be to do the kinds of things we need to do in the varied use cases we have

Great! Eager to hear your opinions.

That said, we do find a few things problematic and I'll try and explain why and demonstrate with code in a separate PR

Would it help if this is merged or your PR would be orthogonal to these changes?

jbrisbin · 2015-03-26T16:33:08Z

I think these changes will be too different to do much in the way of a merge. What I hope will happen is that we discuss the pros and cons of both PRs on their own merits and then prepare another PR that iterates on those ideas. That PR would then be merged to provide the next round, etc...

NiteshKant · 2015-03-26T16:38:20Z

@jbrisbin sounds great!

Based on the various discussions on github, there is no agreement as of now on how to do flush and interception. Removing them for now so that we can iterate on this code to add those features incrementally. Additional changes: - Update rx-reactive-streams version. - Rename `Connection` to `TcpConnection`

NiteshKant · 2015-04-23T18:07:59Z

I have removed interception and flush semantics from this PR.

smaldini · 2015-04-24T12:46:53Z

ripc-protocol-tcp/src/main/java/io/ripc/protocol/tcp/TcpConnection.java

+ * @param <R> The type of objects read from this connection.
+ * @param <W> The type of objects written to this connection.
+ */
+public interface TcpConnection<R, W> extends Publisher<R> {


Why having a separate tcp and ripc-core ? I would suggest just removing all Tcp* prefixes and merge core and tcp protocol. Looking at the artifacts there is nothing specific yet to Tcp. Same for Server and Client, does it have to be in separate module ? Even in a case of File streaming I could see the same abstraction working as well.

Why having a separate tcp and ripc-core

Have a look at issue #18 where we are suggesting a different project structure.
ripc-core would have protocol neutral classes and different protocols will be different projects.

Looking at the artifacts there is nothing specific yet to Tcp

The implementations are TCP specific. Connection concept is protocol specific (connection based protocols). UDP would not have a connection as an example.

Same for Server and Client, does it have to be in separate module?

There is no client as of now. Yeah, they should be in a single module/project per protocol.

Even in a case of File streaming I could see the same abstraction working as well.

You mean file streaming over TCP, HTTP?

Think about the channel from Netty then or from Java NIO, there is absolutely no need to replicate write for every sort of IO, it's not a big deal but its an API question. It would be like saying we have different type of Subscriber in Reactive Stream for Synchronous and Asynchronous ones. UDP is also handled through Channel. So maybe Channel is a better name overall if we want a common denominator.
At this module level we are API centric not Application centric anyway IMO, feel free to name it RxTcpConnection in rxjava tho if you bring specific value to this.

You mean file streaming over TCP, HTTP?
No, I am thinking about reading and writing a File. E.g. Chronicle or other File Memory Mapped IPC used in high throughput apps.

Although I'm not saying specific protocol don't have their modules, but we should have a common notion living in ripc core, named Connection or Channel or Pipe or Flow or IO or whatever.
If we find anything specific to TCP in addition to write and subscribe, e.g. UDP or 0MQ specific helpers they will certainly be able to extend this common artifact. It will bring much more value to the core and a bunch of candidates protocol won't even bother going that far in their first iteration. RIPC is about connecting and moving data between processes so we must make sure the core interface represents that. Server and Client can also do the same, and you already have this notion in RxNetty (as in reactor-net), another qualifier as generic IMO is Recipient and Sender. This is inherent to any communication.

Besides from the place of the components and the naming, I'm equally satisfied with the current both PR which are converging on most of the concept and I find your implementation quite smart (using the Netty user fired event to subscribe was a nice trick 👍 ). I'm in fact already working at mapping the concept in reactor-net to make the transition later easy to the whole concept. But yes as an API this is also very important to set the right expectations. I find the interception a bit more flexible in the alternative PR we issued but beyond that the concepts is really cool.

I see the disconnect here. I think what you are mentioning is more of a commonality in an implementation i.e. every protocol using a socket/channel to read/write data. What this PR is defining is the client/server API for different protocols to be used by the users of reactive-ipc. From an API point of view, different protocols are very different and atleast while implementing UDP, TCP, HTTP, I have not seen commonalities worth defining a common contract across these protocols. This is the reason I am removing all commonalities across protocols in RxNetty w.r.t. client/server APIs. I would recommend refraining from creating these common abstractions unless we have multiple protocols implemented and we see value in such abstractions.

smaldini · 2015-04-27T21:59:36Z

Currently experimenting with this too with reactor, will update asap but looks nice, although the backpressure thing is not simple, not because of the value but because when requesting from inside netty thread it might clash with the onNext written data (some from a different thread, some from the same than netty which then applies the write directly).

I've aligned the reactor-net GA with the semantics in order to move forward easily in the future.

NiteshKant · 2015-04-28T06:42:00Z

@smaldini I did not implement backpressure in this PR to focus discussion on the API and we can discuss backpressure separately.
Backpressure isn't straight forward and just like you are solving it for reactor, I have solved it for RxNetty, it will be great to discuss the details of the same.

For now, I will merge this PR so that we can iterate upon specifics from here.

Proposal for TCP server at all layers (core, transport & rxjava1)

NiteshKant added the proposal label Mar 20, 2015

NiteshKant force-pushed the master branch from 9da096d to 56b6335 Compare March 23, 2015 21:01

NiteshKant force-pushed the master branch from 56b6335 to 89bc5f1 Compare March 23, 2015 21:05

jbrisbin reviewed Mar 25, 2015
View reviewed changes

benjchristensen mentioned this pull request Mar 27, 2015

Additional considerations around the first PR iteration #20

Closed

Adding a short-circuit interception sample.

70956d4

NiteshKant mentioned this pull request Mar 31, 2015

Multiplexing TCP Writes #21

Closed

rstoyanchev mentioned this pull request Apr 7, 2015

Server Interception #23

Open

NiteshKant mentioned this pull request Apr 16, 2015

Should a TcpConnection provide a flush semantic? #8

Open

smaldini reviewed Apr 24, 2015
View reviewed changes

NiteshKant mentioned this pull request Apr 27, 2015

Server Handler Return Type #26

Open

Fixed buid file and removed debug statements

19c8639

NiteshKant added a commit that referenced this pull request Apr 28, 2015

Merge pull request #19 from NiteshKant/master

5bd6143

Proposal for TCP server at all layers (core, transport & rxjava1)

NiteshKant merged commit 5bd6143 into reactive-ipc:master Apr 28, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal for TCP server at all layers (core, transport & rxjava1) #19

Proposal for TCP server at all layers (core, transport & rxjava1) #19

NiteshKant commented Mar 20, 2015

benjchristensen commented Mar 23, 2015

benjchristensen commented Mar 23, 2015

benjchristensen commented Mar 23, 2015

NiteshKant commented Mar 23, 2015

jbrisbin commented Mar 23, 2015

benjchristensen commented Mar 23, 2015

jbrisbin commented Mar 23, 2015

NiteshKant commented Mar 23, 2015

benjchristensen commented Mar 23, 2015

jbrisbin Mar 25, 2015

NiteshKant Mar 25, 2015

jbrisbin Mar 25, 2015

NiteshKant Mar 26, 2015

jbrisbin commented Mar 25, 2015

jbrisbin Mar 25, 2015

NiteshKant Mar 25, 2015

jbrisbin commented Mar 25, 2015

NiteshKant commented Mar 25, 2015

jbrisbin commented Mar 26, 2015

NiteshKant commented Mar 26, 2015

jbrisbin commented Mar 26, 2015

NiteshKant commented Mar 26, 2015

NiteshKant commented Apr 23, 2015

smaldini Apr 24, 2015

NiteshKant Apr 25, 2015

smaldini Apr 25, 2015

smaldini Apr 25, 2015

NiteshKant Apr 27, 2015

smaldini commented Apr 27, 2015

NiteshKant commented Apr 28, 2015


		import org.reactivestreams.Publisher;

		public interface TcpHandler<R, W> {

Proposal for TCP server at all layers (core, transport & rxjava1) #19

Proposal for TCP server at all layers (core, transport & rxjava1) #19

Conversation

NiteshKant commented Mar 20, 2015

benjchristensen commented Mar 23, 2015

benjchristensen commented Mar 23, 2015

benjchristensen commented Mar 23, 2015

NiteshKant commented Mar 23, 2015

jbrisbin commented Mar 23, 2015

benjchristensen commented Mar 23, 2015

jbrisbin commented Mar 23, 2015

NiteshKant commented Mar 23, 2015

benjchristensen commented Mar 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrisbin commented Mar 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrisbin commented Mar 25, 2015

NiteshKant commented Mar 25, 2015

jbrisbin commented Mar 26, 2015

NiteshKant commented Mar 26, 2015

jbrisbin commented Mar 26, 2015

NiteshKant commented Mar 26, 2015

NiteshKant commented Apr 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smaldini commented Apr 27, 2015

NiteshKant commented Apr 28, 2015