You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@qpid.apache.org by rajith attapattu <ra...@gmail.com> on 2012/09/21 17:52:36 UTC

Re: Review Request: Implement a ListMessage interface to accept amqp/list messages as java.util.List

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6816/
-----------------------------------------------------------

(Updated Sept. 21, 2012, 3:52 p.m.)

Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.

Changes
-------

Submitting the original patch provided by Siddesh with the following modifications.

1. Improved error handling code. Added a check to verify that all elements in the list are of the same type.
2. Stream messages are now encoded as an AMQP list. The legacy behaviour can be restored using a connection arg or a jvm arg, similar to the way we handle map messages.
3. Add tests to cover the List, Stream and Map interface as well as NestedLists.

Description
-------

We have a need to support amqp encoded list messages that are understood across all language clients. In JMS this is a bit difficult as there is no direct list message interface like the map message.
I wanted to introduce a way of getting list message support while using only JMS interfaces.

The list message support is implemented as a special case of Map message, where the only entry is a list (it can be nested lists as well).
The application notifies the client of their intention via a message header.
MapMessage m = _session.createMapMessage();
m.setBooleanProperty("encode-as-list", true);
m.setObject("amqp/list", myList);
_producer.send(m);

The resulting message body will be encoded as an AMQP 0-10 list. A C++ or python client can then directly read it as a list message.

On the receiving side, it will be presented as a MapMessage with a single entry retrieved using "amqp/list".
List list = (List)m.getObject("amqp/list");

Please feel free to provide feedback, including alternative ways of implementing this.
All feedback are most welcomed!

This addresses bug QPID-3906.
https://issues.apache.org/jira/browse/QPID-3906

Diffs (updated)
-----

http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1388554
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1388554
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/JMSStreamMessage.java 1388554
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/MessageFactoryRegistry.java 1388554
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/ConnectionURL.java 1388554
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/Session.java 1388554
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1388554

Diff: https://reviews.apache.org/r/6816/diff/

Testing
-------

AMQPEncodedListMessageTest is added to cover several cases.

Thanks,

rajith attapattu

Re: Review Request: Implement a ListMessage interface to accept amqp/list messages as java.util.List

Posted by Gordon Sim <gs...@redhat.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6816/#review11830
-----------------------------------------------------------


Is this missing the added files?

- Gordon Sim


On Sept. 21, 2012, 3:52 p.m., rajith attapattu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6816/
> -----------------------------------------------------------
> 
> (Updated Sept. 21, 2012, 3:52 p.m.)
> 
> 
> Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
> 
> 
> Description
> -------
> 
> We have a need to support amqp encoded list messages that are understood across all language clients. In JMS this is a bit difficult as there is no direct list message interface like the map message.
> I wanted to introduce a way of getting list message support while using only JMS interfaces.
> 
> The list message support is implemented as a special case of Map message, where the only entry is a list (it can be nested lists as well).
> The application notifies the client of their intention via a message header.
> MapMessage m = _session.createMapMessage();
> m.setBooleanProperty("encode-as-list", true);
> m.setObject("amqp/list", myList);
> _producer.send(m);
> 
> The resulting message body will be encoded as an AMQP 0-10 list. A C++ or python client can then directly read it as a list message.
> 
> On the receiving side, it will be presented as a MapMessage with a single entry retrieved using "amqp/list".
> List list = (List)m.getObject("amqp/list");
> 
> Please feel free to provide feedback, including alternative ways of implementing this.
> All feedback are most welcomed!
> 
> 
> This addresses bug QPID-3906.
>     https://issues.apache.org/jira/browse/QPID-3906
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1388554 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1388554 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/JMSStreamMessage.java 1388554 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/MessageFactoryRegistry.java 1388554 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/ConnectionURL.java 1388554 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/Session.java 1388554 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1388554 
> 
> Diff: https://reviews.apache.org/r/6816/diff/
> 
> 
> Testing
> -------
> 
> AMQPEncodedListMessageTest is added to cover several cases.
> 
> 
> Thanks,
> 
> rajith attapattu
> 
>

Re: Review Request: Implement a ListMessage interface to accept amqp/list messages as java.util.List

Posted by rajith attapattu <ra...@gmail.com>.

(Updated Sept. 28, 2012, 5:15 p.m.)

Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.

Changes
-------

Made the following changes.
1. Switched the default encoding to legacy format.
2. It now allows elements of different types.
3. Added several unit tests in favour of a system test.

Description
-------

The resulting message body will be encoded as an AMQP 0-10 list. A C++ or python client can then directly read it as a list message.

On the receiving side, it will be presented as a MapMessage with a single entry retrieved using "amqp/list".
List list = (List)m.getObject("amqp/list");

Please feel free to provide feedback, including alternative ways of implementing this.
All feedback are most welcomed!

This addresses bug QPID-3906.
https://issues.apache.org/jira/browse/QPID-3906

Diffs (updated)
-----

http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1391510
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1391510
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQPEncodedListMessage.java PRE-CREATION
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQPEncodedListMessageFactory.java PRE-CREATION
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/JMSStreamMessage.java 1391510
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/MessageFactoryRegistry.java 1391510
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/ConnectionURL.java 1391510
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/ListMessage.java PRE-CREATION
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/Session.java 1391510
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/test/java/org/apache/qpid/client/AMQConnectionUnitTest.java 1391510
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/test/java/org/apache/qpid/client/AMQSession_0_10Test.java 1391510
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1391510

Diff: https://reviews.apache.org/r/6816/diff/

Testing
-------

AMQPEncodedListMessageTest is added to cover several cases.

Thanks,

rajith attapattu

Re: Review Request: Implement a ListMessage interface to accept amqp/list messages as java.util.List

Posted by rajith attapattu <ra...@gmail.com>.


> On Sept. 25, 2012, 9:38 a.m., Robbie Gemmell wrote:
> > "1. Improved error handling code. Added a check to verify that all elements in the list are of the same type."
> > 
> > Why restrict the elements to a single type? I don't recall JMS placing such a restriction on Map or Stream messages, so why would we?
> > 
> > "2. Stream messages are now encoded as an AMQP list. The legacy behaviour can be restored using a connection arg or a jvm arg, similar to the way we handle map messages."
> > 
> > If the original patch didn't do this, why does this one need to do so? This seems like a separate issue, and something that should be discussed on the dev list before implementation. For example, it may make sense to ship the client with the new format support but the default set the other way, and then flip it in a future release, such that support is present before it becomes the default and people don't need to upgrade all their clients at the same time in order to actually use it, and in the meantime anyone who does want to do so can use the system property.
> > 
> > "3. Add tests to cover the List, Stream and Map interface as well as NestedLists."
> > 
> > I don't see any tests at all in the latest diff, and I know from our email discussion that they still aren't unit tests. This code should be amongst the easier bits of the client to unit test, and it needs unit testing far more than it needs system tests.

1.Gordon also had the same comment about the restriction. I will be removing this.

2. The original patch required the application to use a non standard interface if it needed list message support.
   This approach (just like map messaging) will allow the user to encode a message as amqp/list while using the standard Stream Message interface.

   However I agree with you about switching the default, allowing time for people to make a more manageable upgrade. We could look to switch it around for 0.22 release.

3. I missed the Test class from my diff. I will have to rework the test anyways to accomadate #1. I will also add unit tests.


- rajith


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6816/#review11884
-----------------------------------------------------------


On Sept. 24, 2012, 2:24 p.m., rajith attapattu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6816/
> -----------------------------------------------------------
> 
> (Updated Sept. 24, 2012, 2:24 p.m.)
> 
> 
> Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
> 
> 
> Description
> -------
> 
> We have a need to support amqp encoded list messages that are understood across all language clients. In JMS this is a bit difficult as there is no direct list message interface like the map message.
> I wanted to introduce a way of getting list message support while using only JMS interfaces.
> 
> The list message support is implemented as a special case of Map message, where the only entry is a list (it can be nested lists as well).
> The application notifies the client of their intention via a message header.
> MapMessage m = _session.createMapMessage();
> m.setBooleanProperty("encode-as-list", true);
> m.setObject("amqp/list", myList);
> _producer.send(m);
> 
> The resulting message body will be encoded as an AMQP 0-10 list. A C++ or python client can then directly read it as a list message.
> 
> On the receiving side, it will be presented as a MapMessage with a single entry retrieved using "amqp/list".
> List list = (List)m.getObject("amqp/list");
> 
> Please feel free to provide feedback, including alternative ways of implementing this.
> All feedback are most welcomed!
> 
> 
> This addresses bug QPID-3906.
>     https://issues.apache.org/jira/browse/QPID-3906
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1389396 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1389396 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQPEncodedListMessage.java PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQPEncodedListMessageFactory.java PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/JMSStreamMessage.java 1389396 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/MessageFactoryRegistry.java 1389396 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/ConnectionURL.java 1389396 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/ListMessage.java PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/Session.java 1389396 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1389396 
> 
> Diff: https://reviews.apache.org/r/6816/diff/
> 
> 
> Testing
> -------
> 
> AMQPEncodedListMessageTest is added to cover several cases.
> 
> 
> Thanks,
> 
> rajith attapattu
> 
>

Re: Review Request: Implement a ListMessage interface to accept amqp/list messages as java.util.List

Posted by Robbie Gemmell <ro...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6816/#review11884
-----------------------------------------------------------


"1. Improved error handling code. Added a check to verify that all elements in the list are of the same type."

Why restrict the elements to a single type? I don't recall JMS placing such a restriction on Map or Stream messages, so why would we?

"2. Stream messages are now encoded as an AMQP list. The legacy behaviour can be restored using a connection arg or a jvm arg, similar to the way we handle map messages."

If the original patch didn't do this, why does this one need to do so? This seems like a separate issue, and something that should be discussed on the dev list before implementation. For example, it may make sense to ship the client with the new format support but the default set the other way, and then flip it in a future release, such that support is present before it becomes the default and people don't need to upgrade all their clients at the same time in order to actually use it, and in the meantime anyone who does want to do so can use the system property.

"3. Add tests to cover the List, Stream and Map interface as well as NestedLists."

I don't see any tests at all in the latest diff, and I know from our email discussion that they still aren't unit tests. This code should be amongst the easier bits of the client to unit test, and it needs unit testing far more than it needs system tests.

- Robbie Gemmell


On Sept. 24, 2012, 2:24 p.m., rajith attapattu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6816/
> -----------------------------------------------------------
> 
> (Updated Sept. 24, 2012, 2:24 p.m.)
> 
> 
> Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.
> 
> 
> Description
> -------
> 
> We have a need to support amqp encoded list messages that are understood across all language clients. In JMS this is a bit difficult as there is no direct list message interface like the map message.
> I wanted to introduce a way of getting list message support while using only JMS interfaces.
> 
> The list message support is implemented as a special case of Map message, where the only entry is a list (it can be nested lists as well).
> The application notifies the client of their intention via a message header.
> MapMessage m = _session.createMapMessage();
> m.setBooleanProperty("encode-as-list", true);
> m.setObject("amqp/list", myList);
> _producer.send(m);
> 
> The resulting message body will be encoded as an AMQP 0-10 list. A C++ or python client can then directly read it as a list message.
> 
> On the receiving side, it will be presented as a MapMessage with a single entry retrieved using "amqp/list".
> List list = (List)m.getObject("amqp/list");
> 
> Please feel free to provide feedback, including alternative ways of implementing this.
> All feedback are most welcomed!
> 
> 
> This addresses bug QPID-3906.
>     https://issues.apache.org/jira/browse/QPID-3906
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1389396 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1389396 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQPEncodedListMessage.java PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQPEncodedListMessageFactory.java PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/JMSStreamMessage.java 1389396 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/MessageFactoryRegistry.java 1389396 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/ConnectionURL.java 1389396 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/ListMessage.java PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/Session.java 1389396 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1389396 
> 
> Diff: https://reviews.apache.org/r/6816/diff/
> 
> 
> Testing
> -------
> 
> AMQPEncodedListMessageTest is added to cover several cases.
> 
> 
> Thanks,
> 
> rajith attapattu
> 
>

Re: Review Request: Implement a ListMessage interface to accept amqp/list messages as java.util.List

Posted by rajith attapattu <ra...@gmail.com>.

(Updated Sept. 24, 2012, 2:24 p.m.)

Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Keith Wall.

Changes
-------

Added the missing files. Thx Gordon for pointing it out.

Description
-------

The resulting message body will be encoded as an AMQP 0-10 list. A C++ or python client can then directly read it as a list message.

On the receiving side, it will be presented as a MapMessage with a single entry retrieved using "amqp/list".
List list = (List)m.getObject("amqp/list");

Please feel free to provide feedback, including alternative ways of implementing this.
All feedback are most welcomed!

This addresses bug QPID-3906.
https://issues.apache.org/jira/browse/QPID-3906

Diffs (updated)
-----

http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java 1389396
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1389396
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQPEncodedListMessage.java PRE-CREATION
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQPEncodedListMessageFactory.java PRE-CREATION
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/JMSStreamMessage.java 1389396
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/MessageFactoryRegistry.java 1389396
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/ConnectionURL.java 1389396
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/ListMessage.java PRE-CREATION
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/Session.java 1389396
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1389396

Diff: https://reviews.apache.org/r/6816/diff/

Testing
-------

AMQPEncodedListMessageTest is added to cover several cases.

Thanks,

rajith attapattu