You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Gordon Sim <gs...@redhat.com> on 2012/11/07 15:47:27 UTC
Review Request: Messenger API and implementation
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7934/
-----------------------------------------------------------
Review request for qpid, Robbie Gemmell, rajith attapattu, Keith Wall, Rafael Schloming, and Rob Godfrey.
Description
-------
Added Messenger API and impl. Probably need some sort of factory if we keep this split to avoid direct construction of the impl class.
Doesn't yet support acknowledgements (thats coming next, though).
This addresses bug PROTON-118.
https://issues.apache.org/jira/browse/PROTON-118
Diffs
-----
/proton/trunk/proton-j/src/main/java/org/apache/qpid/proton/messenger/Messenger.java PRE-CREATION
/proton/trunk/proton-j/src/main/java/org/apache/qpid/proton/messenger/impl/MessengerImpl.java PRE-CREATION
/proton/trunk/proton-j/src/main/scripts/proton.py 1406092
/proton/trunk/tests/proton_tests/messenger.py 1406092
Diff: https://reviews.apache.org/r/7934/diff/
Testing
-------
In conjunction with the Driver changes (submitted as separate patch for same JIRA) and the patch attached to PROTON-120, the common test suite has been run with no new failures. The messenger tests have been enabled and the shim modified. The acknowledgement related messenger tests fail as that is not yet implemented, send&receive test fails for messages over 10k (I think that may be an engine issue but will dig some more).
Thanks,
Gordon Sim
Re: Review Request: Messenger API and implementation
Posted by Gordon Sim <gs...@redhat.com>.
> On Dec. 12, 2012, 3:39 a.m., rajith attapattu wrote:
> > 1. It would be good to have Java doc for the API classes.
> >
> > 2. As you suggest a factory approach might be good especially given that Phil and Keith are working on a version on top of the C impl via swig.
> >
> > 3. Given above, it might be better to have Message.java as an interface and move the current impl into a MessageImpl.java under the impl directory.
Re (1.) agree completely. Will try and add some in ASAP.
Re (2.), the key point I think is that the same approach should be taken in engine, driver and messenger for the sake of consistency.
I'm less concerned about (3.) as the message class is used by all engine and messenger implementations and is different in nature to those. In any case its probably a separate concern from the messenger itself.
> On Dec. 12, 2012, 3:39 a.m., rajith attapattu wrote:
> > /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/impl/MessengerImpl.java, line 66
> > <https://reviews.apache.org/r/7934/diff/3/?file=236677#file236677line66>
> >
> > Perhaps allows this to be configured via a system prop?
> >
> > (not really an issue bcos you auto-expand)
Possibly, yes. I do think we want to be careful about the configurable options we expose. This could indeed be one. I'm not in general terribly happy with this aspect of the implementation at present. I think the engine also does some auto-expanding of some sort. May be worth stepping back and thinking about the best approach in general.
> On Dec. 12, 2012, 3:39 a.m., rajith attapattu wrote:
> > /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/impl/MessengerImpl.java, line 108
> > <https://reviews.apache.org/r/7934/diff/3/?file=236677#file236677line108>
> >
> > Should we not close the connector here?
> > It looks like we are leaking tcp connections here ?
Yes, I think we need an explicit close here.
> On Dec. 12, 2012, 3:39 a.m., rajith attapattu wrote:
> > /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/impl/MessengerImpl.java, line 339
> > <https://reviews.apache.org/r/7934/diff/3/?file=236677#file236677line339>
> >
> > Perhaps we should have a read and write buffer with a sensible default and then allow it to be configured via a system prop.
> >
> > This way it allows an application to provide a reasonable size as it *may* know the message sizes they are expecting.
As above.
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7934/#review14343
-----------------------------------------------------------
On Dec. 10, 2012, 10:29 p.m., Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7934/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2012, 10:29 p.m.)
>
>
> Review request for qpid, rajith attapattu, Rafael Schloming, and Rob Godfrey.
>
>
> Description
> -------
>
> Added Messenger API and impl. Probably need some sort of factory if we keep this split to avoid direct construction of the impl class.
>
>
> This addresses bug PROTON-118.
> https://issues.apache.org/jira/browse/PROTON-118
>
>
> Diffs
> -----
>
> /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/AcceptMode.java PRE-CREATION
> /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/Messenger.java PRE-CREATION
> /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/MessengerException.java PRE-CREATION
> /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/Status.java PRE-CREATION
> /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/Tracker.java PRE-CREATION
> /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/impl/MessengerImpl.java PRE-CREATION
> /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/impl/TrackerImpl.java PRE-CREATION
> /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/impl/TrackerQueue.java PRE-CREATION
> /proton/trunk/proton-j/proton/src/main/scripts/proton.py 1419839
>
> Diff: https://reviews.apache.org/r/7934/diff/
>
>
> Testing
> -------
>
> I have a little bit more work required on the test shim to handle accessing message body as string in python tests (currently hacked around that to get tests passing). Also the 100K and 1M message tests don't pass yet (not sure where the issue is for that).
>
>
> Thanks,
>
> Gordon Sim
>
>
Re: Review Request: Messenger API and implementation
Posted by rajith attapattu <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7934/#review14343
-----------------------------------------------------------
1. It would be good to have Java doc for the API classes.
2. As you suggest a factory approach might be good especially given that Phil and Keith are working on a version on top of the C impl via swig.
3. Given above, it might be better to have Message.java as an interface and move the current impl into a MessageImpl.java under the impl directory.
/proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/impl/MessengerImpl.java
<https://reviews.apache.org/r/7934/#comment30543>
Perhaps allows this to be configured via a system prop?
(not really an issue bcos you auto-expand)
/proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/impl/MessengerImpl.java
<https://reviews.apache.org/r/7934/#comment30542>
Should we not close the connector here?
It looks like we are leaking tcp connections here ?
/proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/impl/MessengerImpl.java
<https://reviews.apache.org/r/7934/#comment30546>
Perhaps we should have a read and write buffer with a sensible default and then allow it to be configured via a system prop.
This way it allows an application to provide a reasonable size as it *may* know the message sizes they are expecting.
- rajith attapattu
On Dec. 10, 2012, 10:29 p.m., Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7934/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2012, 10:29 p.m.)
>
>
> Review request for qpid, rajith attapattu, Rafael Schloming, and Rob Godfrey.
>
>
> Description
> -------
>
> Added Messenger API and impl. Probably need some sort of factory if we keep this split to avoid direct construction of the impl class.
>
>
> This addresses bug PROTON-118.
> https://issues.apache.org/jira/browse/PROTON-118
>
>
> Diffs
> -----
>
> /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/AcceptMode.java PRE-CREATION
> /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/Messenger.java PRE-CREATION
> /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/MessengerException.java PRE-CREATION
> /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/Status.java PRE-CREATION
> /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/Tracker.java PRE-CREATION
> /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/impl/MessengerImpl.java PRE-CREATION
> /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/impl/TrackerImpl.java PRE-CREATION
> /proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/impl/TrackerQueue.java PRE-CREATION
> /proton/trunk/proton-j/proton/src/main/scripts/proton.py 1419839
>
> Diff: https://reviews.apache.org/r/7934/diff/
>
>
> Testing
> -------
>
> I have a little bit more work required on the test shim to handle accessing message body as string in python tests (currently hacked around that to get tests passing). Also the 100K and 1M message tests don't pass yet (not sure where the issue is for that).
>
>
> Thanks,
>
> Gordon Sim
>
>
Re: Review Request: Messenger API and implementation
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7934/
-----------------------------------------------------------
(Updated Dec. 10, 2012, 10:29 p.m.)
Review request for qpid, rajith attapattu, Rafael Schloming, and Rob Godfrey.
Changes
-------
Updated patch that includes support for acknowledgements.
Description (updated)
-------
Added Messenger API and impl. Probably need some sort of factory if we keep this split to avoid direct construction of the impl class.
This addresses bug PROTON-118.
https://issues.apache.org/jira/browse/PROTON-118
Diffs (updated)
-----
/proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/AcceptMode.java PRE-CREATION
/proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/Messenger.java PRE-CREATION
/proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/MessengerException.java PRE-CREATION
/proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/Status.java PRE-CREATION
/proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/Tracker.java PRE-CREATION
/proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/impl/MessengerImpl.java PRE-CREATION
/proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/impl/TrackerImpl.java PRE-CREATION
/proton/trunk/proton-j/proton/src/main/java/org/apache/qpid/proton/messenger/impl/TrackerQueue.java PRE-CREATION
/proton/trunk/proton-j/proton/src/main/scripts/proton.py 1419839
Diff: https://reviews.apache.org/r/7934/diff/
Testing (updated)
-------
I have a little bit more work required on the test shim to handle accessing message body as string in python tests (currently hacked around that to get tests passing). Also the 100K and 1M message tests don't pass yet (not sure where the issue is for that).
Thanks,
Gordon Sim
Re: Review Request: Messenger API and implementation
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7934/
-----------------------------------------------------------
(Updated Nov. 8, 2012, 1:58 p.m.)
Review request for qpid, Robbie Gemmell, rajith attapattu, Keith Wall, Rafael Schloming, and Rob Godfrey.
Changes
-------
Added missing MessengerException class. Corrected bracketing to match current project coding-style more consistently.
Description
-------
Added Messenger API and impl. Probably need some sort of factory if we keep this split to avoid direct construction of the impl class.
Doesn't yet support acknowledgements (thats coming next, though).
This addresses bug PROTON-118.
https://issues.apache.org/jira/browse/PROTON-118
Diffs (updated)
-----
/proton/trunk/proton-j/src/main/java/org/apache/qpid/proton/messenger/Messenger.java PRE-CREATION
/proton/trunk/proton-j/src/main/java/org/apache/qpid/proton/messenger/MessengerException.java PRE-CREATION
/proton/trunk/proton-j/src/main/java/org/apache/qpid/proton/messenger/impl/MessengerImpl.java PRE-CREATION
/proton/trunk/proton-j/src/main/scripts/proton.py 1406092
/proton/trunk/tests/proton_tests/messenger.py 1406092
Diff: https://reviews.apache.org/r/7934/diff/
Testing
-------
In conjunction with the Driver changes (submitted as separate patch for same JIRA) and the patch attached to PROTON-120, the common test suite has been run with no new failures. The messenger tests have been enabled and the shim modified. The acknowledgement related messenger tests fail as that is not yet implemented, send&receive test fails for messages over 10k (I think that may be an engine issue but will dig some more).
Thanks,
Gordon Sim