On 09/09/2009 10:21 PM, Alan Conway wrote:
I just went over the messaging API on trunk, I scribbled some comments
on it diff attached.
Thanks, Alan, all good points!
First, the easiest point to address, Session::getLastConfirmedSent() and
Session::getLastConfirmedAcknowledged() should no longer be in there
(part of an aborted earlier approach) and I'll remove them.
* Session::reject(Message&) -> Message::reject()
I wasn't keen on this, but I can't really justify my dislike so I'll go
ahead and make that change.
* ClientImportExport.h
I viewed these as related to libraries, not namespaces, but I agree the
dependency is not ideal. Are you suggesting that there should be a
separate library for the new API or just that there should be a separate
header file defining those macros for use in headers in the messaging
namespace?
* Address class
The reply-to address for a message must be encoded into the AMQP 0-10
format for transmission and this requires interpreting what the address
refers to (e.g. a 'topic', a 'queue' or some other 0-10 specific
'address'). It is obviously desirable to avoid doing unnecessary work
for every message sent with a reply-to defined, and the Address class
was introduced as the type of the ReplyTo header on a message in order
to hold details of a 'pre-interpreted' address (which may have required
parsing or even lookup on the broker) that can be directly encoded more
quickly.
It may be that the parsing does not add significant overhead and we
could simply remove the Address struct and rely on a simple string. That
would certainly be preferable for simplicity.
* Filter class
Filters are a means of indicating to the server what messages the
receiver is interested in. The filtering is not (in general at least)
done by the client. For this reason I don't think a Filter is 'an object
that filters' but rather a description of the filtering it requires the
server to perform.
At present we use filters to describe the types and details of bindings
for 'topics'.
I'm not sure that having a separate class is necessary however (and the
class itself is rather odd looking, I agree). I think we can probably
merge the filter in to the options for the receiver. I'll try that out
and we can see what it looks like.
* MessageContent & Codec
I agree that the encode/decode methods should not be on the message and
will remove them.
The concept at present is that a message content can be one of two
'structured types' (list or map), or just a sequence of bytes. The
structured types represent the 'logical' structure of the content and
are not intended to imply any particular encoding.
I'm still not sure whether the structured content should be a part of
the message or whether that aspect should be handled separately and am
not totally comfortable with the current form. The reason for having it
'built in' to the message is convenience for the user; they can simply
pass a message whose content is a map of values to be sent and can
receive a message already decoded into a map for processing. Of course
for transmission over the network even the structured types will need to
be encoded into a sequence of bytes and encoding and decoding will
happen 'under the covers' by choosing a particular encoding (ideally in
some configurable manner).
The alternative would be to have message simply represent the bytes to
be sent or received and have the encode/decode and structural
representations kept distinct. That would I think certainly result in a
simpler API. The question is whether there is value in doing this
automatically, and if so whether there is a better way of doing so.
I'll think about this some more and will very much welcome any further
comments or suggestions you may have.
Planning for non-contiguous sequences of bytes is also a good idea, even
if the implementation that exploits it follows later on. Keen to see
your thoughts on that.
* Receiver: capacity (& start/stop), session-level fetch, message listeners
The capacity of a listener is indeed a flow control window. If the
capacity is 0 then messages are not prefetched and are only transferred
from the server in response to an explicit fetch() call.
I think it may be better to remove the start/stop as they aren't really
needed. However at present while the capacity defines the credit window,
the start and stop control whether it is in effect or not (and have no
effect when capacity = 0).
The session level fetch allows you to get messages from any of the
subscriptions created for the session, in order, as they arrive. I think
this is very useful.
In addition to the straightforward pull style of session level fetch,
there is the ability dispatch the next available message to the listener
defined for the subscription it relates to (if a listener was defined).
(See e.g. the topic_listener for an example, though not necessarily the
most illuminating in its current form).
I haven't addressed some of your earlier points on threading but I
haven't forgotten about them either. I hope to respond on that point in
more detail before too long.
I should also - belatedly - revise expectations on completion of the
API. It is not going to be finished by the end of this week. We are
getting close (and I have a few additions that I should get in today or
tomorrow), but more of this sort of review is important as is some
performance evaluation and likely some tuning. I'd like another couple
of weeks polishing and aligning with python and the JMS client.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:[email protected]
