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