search

reactive-ipc / reactive-ipc-jvm

Reactive IPC for the JVM
Apache License 2.0
55 stars 13 forks source link

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

Closed NiteshKant closed 9 years ago

NiteshKant commented 9 years ago

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 commented 9 years ago

@NiteshKant Can you rebase this for consideration to merge?

benjchristensen commented 9 years ago

@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 commented 9 years ago

BTW, once we have a good approach we can migrate the tcp protocol code to its own project as per https://github.com/reactive-ipc/reactive-ipc-jvm/issues/18

NiteshKant commented 9 years ago

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

jbrisbin commented 9 years ago

:+1:

benjchristensen commented 9 years ago

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

jbrisbin commented 9 years ago

Ideally they should be squashed to 1.

NiteshKant commented 9 years ago

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

benjchristensen commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

How would we handle connection close events and reconnects?

NiteshKant commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

@jbrisbin sounds great!

NiteshKant commented 9 years ago

I have removed interception and flush semantics from this PR.

smaldini commented 9 years ago

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 commented 9 years ago

@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.