You are viewing a plain text version of this content. The canonical link for it is here.
Posted to proton@qpid.apache.org by prestona <gi...@git.apache.org> on 2015/05/07 00:33:38 UTC
[GitHub] qpid-proton pull request: Reactor
GitHub user prestona opened a pull request:
https://github.com/apache/qpid-proton/pull/29
Reactor
This is an initial implementation of a proton-j version of the reactor. It comes with samples, unit tests and interoperation tests. It should be good enough to "kick the tyres" but requires further test cases, code cleanup and Javadoc. For the most part, the code is additive to the proton-j codebase.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/prestona/qpid-proton reactor
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/qpid-proton/pull/29.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #29
----
commit e0187017456a4df58df2f1d04b1941d99eacbe10
Author: Adrian Preston <pr...@uk.ibm.com>
Date: 2015-04-17T13:33:11Z
PROTON-881: Initial commit of proton-j reactor implementation
commit 739005e7ca49b0e85873406ea4a6b9392252a1fe
Author: Adrian Preston <pr...@uk.ibm.com>
Date: 2015-04-17T14:34:53Z
PROTON-881: Write an Echo example and get it working
commit 88df5e7490183e01dfc8c63d2cfe3123286e604b
Author: Adrian Preston <pr...@uk.ibm.com>
Date: 2015-04-17T16:55:27Z
PROTON-881: Add a Cat example.
commit cd09de66362580f0c5ceab464d71c7ad4300b517
Author: Adrian Preston <pr...@uk.ibm.com>
Date: 2015-04-21T15:23:12Z
PROTON-881: Add a Send example, and supporting changes in the reactor.
commit 0ac98e76be51fbd890613518541364301b191cc2
Author: Adrian Preston <pr...@uk.ibm.com>
Date: 2015-04-22T20:35:36Z
PROTON-881: Add Recv sample and required code changes / additions to the reactor
commit b6e18b5a35da5865fe0ae5fd9e161a04170e9750
Author: Adrian Preston <pr...@uk.ibm.com>
Date: 2015-04-29T22:57:47Z
PROTON-881: Rough first pass at a proton-j -> proton-c interop. test.
This adds a Python test case which starts both a Python (proton-c)
reactor and also spawns a JVM running a (proton-j) reactor. The
expected outcome is that the proton-j reactor is able to send a
message to the proton-c reactor.
Right now, there are a lot of rough edges on both the Pyton and
Java sides of this test case.
commit 2e6f5cdd1754b266e81afbc49ae4333a75287d57
Author: Adrian Preston <pr...@uk.ibm.com>
Date: 2015-04-30T12:58:28Z
PROTON-881: Tidy up proton-j to proton-c reactor interop tests
commit 7faa7e2322c4782b9acc173194dc8bb1887a5a89
Author: Adrian Preston <pr...@uk.ibm.com>
Date: 2015-04-30T13:51:46Z
PROTON-881: Only try interoperation tests with proton-j if proton-j has been built!
commit 1eb41f603b0a4c5da9c686af1369837e7c6f2184
Author: Adrian Preston <pr...@uk.ibm.com>
Date: 2015-05-01T14:36:00Z
PROTON-881: Add reactor interop tests that send messages from proton-c to proton-j
commit 51529f675a11f49089e4a50a6bbced3955c26d63
Author: Adrian Preston <pr...@uk.ibm.com>
Date: 2015-05-01T14:55:07Z
PROTON-881: Tidy up Selectable to remove bits of a c style record implementation
commit 5748bb9880432bab64c42d34f4d7277163077427
Author: Adrian Preston <pr...@uk.ibm.com>
Date: 2015-05-02T21:30:40Z
PROTON-881: Add error handling for "accept socket connection" path through Acceptor
Adds error handling for various problems that can occur when accepting
a connection (to a listening socket). Also adds unittests that cover
these cases.
commit e9d4a78d294f08e7eb1bd7a3f28bd3f97ce6b9df
Author: Adrian Preston <pr...@uk.ibm.com>
Date: 2015-05-04T20:01:19Z
PROTON-881: Add reactor unit tests based on those in proton-c/src/tests/reactor.c
commit d6c4ba7bb18d3b8ce02a8dcd52c0f1e569694bb7
Author: Adrian Preston <pr...@uk.ibm.com>
Date: 2015-05-04T20:57:53Z
PROTON-881: Add code to release resources (e.g. sockets, selectors, etc.)
The proton-j codebase does not, generally, implement cleanup logic in
the same way as proton-c (explicit reference counting) and for some
resources (such as sockets, selectors, etc.) cannot always way for
garbage collection to finalize objects. This commit adds a 'free'
method to a number of classes. Calling this closes any Java resources
held by the class. The reactor also has a 'free' method, which frees
both the reactor and any children that the reactor has.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] qpid-proton pull request: Reactor
Posted by Rafael Schloming <rh...@alum.mit.edu>.
On Thu, May 7, 2015 at 7:29 AM, gemmellr <gi...@git.apache.org> wrote:
> Github user gemmellr commented on a diff in the pull request:
>
> https://github.com/apache/qpid-proton/pull/29#discussion_r29843571
>
> --- Diff:
> proton-j/src/main/java/org/apache/qpid/proton/engine/impl/EventImpl.java ---
> @@ -157,12 +165,51 @@ public void dispatch(Handler handler)
> case TRANSPORT_CLOSED:
> handler.onTransportClosed(this);
> break;
> + case REACTOR_FINAL:
> + handler.onReactorFinal(this);
> + break;
> + case REACTOR_QUIESCED:
> + handler.onReactorQuiesced(this);
> + break;
> + case REACTOR_INIT:
> + handler.onReactorInit(this);
> + break;
> + case SELECTABLE_ERROR:
> + handler.onSelectableError(this);
> + break;
> + case SELECTABLE_EXPIRED:
> + handler.onSelectableExpired(this);
> + break;
> + case SELECTABLE_FINAL:
> + handler.onSelectableFinal(this);
> + break;
> + case SELECTABLE_INIT:
> + handler.onSelectableInit(this);
> + break;
> + case SELECTABLE_READABLE:
> + handler.onSelectableReadable(this);
> + break;
> + case SELECTABLE_UPDATED:
> + handler.onSelectableWritable(this);
> + break;
> + case SELECTABLE_WRITABLE:
> + handler.onSelectableWritable(this);
> + break;
> + case TIMER_TASK:
> + handler.onTimerTask(this);
> + break;
> default:
> handler.onUnhandled(this);
> break;
> }
> +
> + Iterator<Handler> children = handler.children();
> --- End diff --
>
> Commenting here to avoid spamming the semi-unrelated JIRA mentioned
> linked with the other PR that got merged.
>
> I have only skimmed this PR, since I haven't got much of experience of
> the reactor code in any of the other languages, so I'm not sure what many
> things are actually meant to do.
>
> It felt a little strange at times that some of the Connection etc
> objects now have some very reactor specific methods even though you might
> not be using the reactor with them (i.e all existing use), but I can
> certainly live with that if thats how it works elsewhere, and they
> presumably dont hurt anything if you dont use them.
>
There may be a way to reduce the coupling a little bit there (I'm currently
looking into that), but in the end it simply becomes awkward to write event
handlers without being able to navigate from all the various engine objects
back to the reactor that holds them.
> I did spot this 1 specific bit though, where I think it would be nice
> to optimise for what anyone using this currently will be doing. Existing
> handlers wont have any children, and that would currently mean that every
> time you dispatch an event you will create a pointless iterator, so it
> would be nice to avoid that in the case of no children.
>
Agreed. I need to tweak event dispatch to support some other scenarios, so
I'll probably be messing with that code anyways. I'll make sure it doesn't
create extraneous objects when I do.
--Rafael
[GitHub] qpid-proton pull request: Reactor
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/29#discussion_r29843571
--- Diff: proton-j/src/main/java/org/apache/qpid/proton/engine/impl/EventImpl.java ---
@@ -157,12 +165,51 @@ public void dispatch(Handler handler)
case TRANSPORT_CLOSED:
handler.onTransportClosed(this);
break;
+ case REACTOR_FINAL:
+ handler.onReactorFinal(this);
+ break;
+ case REACTOR_QUIESCED:
+ handler.onReactorQuiesced(this);
+ break;
+ case REACTOR_INIT:
+ handler.onReactorInit(this);
+ break;
+ case SELECTABLE_ERROR:
+ handler.onSelectableError(this);
+ break;
+ case SELECTABLE_EXPIRED:
+ handler.onSelectableExpired(this);
+ break;
+ case SELECTABLE_FINAL:
+ handler.onSelectableFinal(this);
+ break;
+ case SELECTABLE_INIT:
+ handler.onSelectableInit(this);
+ break;
+ case SELECTABLE_READABLE:
+ handler.onSelectableReadable(this);
+ break;
+ case SELECTABLE_UPDATED:
+ handler.onSelectableWritable(this);
+ break;
+ case SELECTABLE_WRITABLE:
+ handler.onSelectableWritable(this);
+ break;
+ case TIMER_TASK:
+ handler.onTimerTask(this);
+ break;
default:
handler.onUnhandled(this);
break;
}
+
+ Iterator<Handler> children = handler.children();
--- End diff --
Commenting here to avoid spamming the semi-unrelated JIRA mentioned linked with the other PR that got merged.
I have only skimmed this PR, since I haven't got much of experience of the reactor code in any of the other languages, so I'm not sure what many things are actually meant to do.
It felt a little strange at times that some of the Connection etc objects now have some very reactor specific methods even though you might not be using the reactor with them (i.e all existing use), but I can certainly live with that if thats how it works elsewhere, and they presumably dont hurt anything if you dont use them.
I did spot this 1 specific bit though, where I think it would be nice to optimise for what anyone using this currently will be doing. Existing handlers wont have any children, and that would currently mean that every time you dispatch an event you will create a pointless iterator, so it would be nice to avoid that in the case of no children.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: Reactor
Posted by prestona <gi...@git.apache.org>.
Github user prestona commented on the pull request:
https://github.com/apache/qpid-proton/pull/29#issuecomment-99640540
Closing. Will submit again against proton-j-reactor
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: Reactor
Posted by prestona <gi...@git.apache.org>.
Github user prestona closed the pull request at:
https://github.com/apache/qpid-proton/pull/29
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: Reactor
Posted by rhs <gi...@git.apache.org>.
Github user rhs commented on the pull request:
https://github.com/apache/qpid-proton/pull/29#issuecomment-99635551
Given my recent rant about new development on master, let's try pulling this in on a branch first. I've create the proton-j-reactor branch. If you can resubmit against that I'll pull it in and raise the issue on the 0.10 release thread of whether we want to include it in the release.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---