You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Alan Conway <ac...@redhat.com> on 2009/09/09 23:21:21 UTC
Re: [c++] latest messaging api patch
I just went over the messaging API on trunk, I scribbled some comments on it
diff attached.
Re: [c++] latest messaging api patch
Posted by Gordon Sim <gs...@redhat.com>.
On 09/10/2009 05:29 PM, Alan Conway wrote:
> On 09/10/2009 08:14 AM, Gordon Sim wrote:
>> On 09/09/2009 10:21 PM, Alan Conway wrote:
>> * 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?
>
> They really are about libraries so I guess its OK. I was mostly bothered
> by the #include "qpid/client/...", I'd like to see a clean break with
> the old API. It's not a critical point.
Its easy enough to add in a new header with effectively the same macros.
Alternatively I guess we could move ClientImportExport.h up a level
alongside CommonImportExport.h.
>> * 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.
>
> I'd be inclined make Address a PIMPL so we can do whatever we want with
> regard to pre-parsing etc. We can use the flyweight pattern to avoid
> allocations and make them light weight if we need to.
Ok, I'll see what I can do there.
>> * 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.
>>
>
> How about this: a Message is a container for data which internally might
> be non-contiguous etc. But Message itself provides no access to the data.
>
> A set of *View classes provide "views" of the message data.
>
> So you'd have StringView to view the message as a contiguous string,
> MapView to parse the message as a map etc.
>
> Message m;
> std::string x = StringView(m);
> MapView mm(m); // throw if cant be parsed as map.
> cout << mm["foo"]
>
> If/when we support multi-buffer messages we can add a BuffersView which
> allows the user to view the underlying buffers directly with no copying.
>
> Views are read-only and may or may not copy data from the message so
> modifying the message may invalidate some types of view (like modifying
> a std::collection may invalidate an iterator)
>
> When we want to create or modify a message we use corresponding Content
> classes. Views are read-only, Contents are read write. You can use
> Contents in two ways
>
> 1. pass to message constructor or assign
> Message m(StringContent("foo"));
> m = StringContent("bar")
>
> 2. modify existing message
> Message m;
> MapContent mm(m);
> mm["foo"] = ...;
In this second example, at what point would the map be encoded?
> The Message API will have constructor/assign etc. for the Content base
> class, we'll figure out the type under the covers. That means we can
> expand the range of content types without any change to the Message API.
>
> If/when we support multi-buffer messages a BuffersContent will allow the
> user to provide pointers to their own buffers, extend a message by
> adding buffers etc. We may provide some options around avoiding copies,
> the default should be that we copy though as there are lifcycle issues
> to work out if we don't copy.
>
> We could also provide view/content that implement std::streambuf for
> efficient integration with iostreams.
>
> I'm in two minds as to whether View and Content are separate classes or
> should be combined into just one read-write Content class. The
> motivation for separation is const correctness - FooView(const Message&)
> but FooContent(Message&). Probably in most cases the Content would
> inherit from the View for the read operations.
I like the idea here! I'm not sure I fully see how to implement it yet
but will mull it over a bit and perhaps come back with some questions.
>> * 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).
> What does "in effect" mean? How does stopped the stopped state differ
> capacity==0?
In effect means 'actually granted'. I.e. I can set a receiver to have a
window of 50 messages. Only when I start() the receiver is that credit
granted. If I stop() the receiver then the broker is asked to reset
granted credit to 0. (I can then call start() again to get messages
flowing again).
When the capacity is 0 start and stop don't have any effect.
Like I say, I think they may well not be needed and could be removed.
>> 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.
>
> Agreed. What are the rules if a message arrives when there are any/all
> of receiver::fetch, session::fetch and receiver listener active?
If you call Receiver::fetch() concurrently from more than one thread
then each thread will get some set of the available messages (no strict
round robin is supported). The situation is pretty much identical if you
have Session::fetch() called concurrently as well. All these calls are
serialised at the session level.
Listeners are currently only relevant if you call Session::dispatch().
This will in effect fetch the next available message for any receiver
that has a listener set and then pass that message to the listener. If
called concurrently it would behave similarly to Session::fetch().
>> 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 would be inclined to say something like this: once a listener is
> provided to a receiver, its received() may be called at any time in an
> unspecified thread until the receiver is destroyed or the listener is
> un-associated from it (do we even allow that?) Calls to received() are
> guaranteed to be sequential but they're not guaranteed to be all in the
> same thread.
>
> That gives us a workable API behind which we can implement a sensible
> client-side thread pool initially and add more user-controlled threading
> options going forward. I would be inclined to avoid any other threading
> knobs and dials (e.g. start() stop() run() type stuff) as that wires
> assumptions about threading into the API and restricts what we can do.
Agreed and it is these methods I've tried to avoid. The current
stop()/start() calls don't start or stop threads in anyway; in fact the
new API implementation does not start any threads at all (it does
however use the qpid::client::Connection class which for now at least
starts its own IO thread).
(I wasn't mad on the names at first as I felt it implied something like
a thread being started. If I get rid of them that will end the confusion!).
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: [c++] latest messaging api patch
Posted by Alan Conway <ac...@redhat.com>.
On 09/10/2009 08:14 AM, Gordon Sim wrote:
> On 09/09/2009 10:21 PM, Alan Conway wrote:
> * 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?
They really are about libraries so I guess its OK. I was mostly bothered by the
#include "qpid/client/...", I'd like to see a clean break with the old API. It's
not a critical point.
> * 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.
I'd be inclined make Address a PIMPL so we can do whatever we want with regard
to pre-parsing etc. We can use the flyweight pattern to avoid allocations and
make them light weight if we need to.
> * 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.
>
How about this: a Message is a container for data which internally might be
non-contiguous etc. But Message itself provides no access to the data.
A set of *View classes provide "views" of the message data.
So you'd have StringView to view the message as a contiguous string, MapView to
parse the message as a map etc.
Message m;
std::string x = StringView(m);
MapView mm(m); // throw if cant be parsed as map.
cout << mm["foo"]
If/when we support multi-buffer messages we can add a BuffersView which allows
the user to view the underlying buffers directly with no copying.
Views are read-only and may or may not copy data from the message so modifying
the message may invalidate some types of view (like modifying a std::collection
may invalidate an iterator)
When we want to create or modify a message we use corresponding Content classes.
Views are read-only, Contents are read write. You can use Contents in two ways
1. pass to message constructor or assign
Message m(StringContent("foo"));
m = StringContent("bar")
2. modify existing message
Message m;
MapContent mm(m);
mm["foo"] = ...;
The Message API will have constructor/assign etc. for the Content base class,
we'll figure out the type under the covers. That means we can expand the range
of content types without any change to the Message API.
If/when we support multi-buffer messages a BuffersContent will allow the user to
provide pointers to their own buffers, extend a message by adding buffers etc.
We may provide some options around avoiding copies, the default should be that
we copy though as there are lifcycle issues to work out if we don't copy.
We could also provide view/content that implement std::streambuf for efficient
integration with iostreams.
I'm in two minds as to whether View and Content are separate classes or should
be combined into just one read-write Content class. The motivation for
separation is const correctness - FooView(const Message&) but
FooContent(Message&). Probably in most cases the Content would inherit from the
View for the read operations.
> * 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).
What does "in effect" mean? How does stopped the stopped state differ 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.
Agreed. What are the rules if a message arrives when there are any/all of
receiver::fetch, session::fetch and receiver listener active?
> 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 would be inclined to say something like this: once a listener is provided to a
receiver, its received() may be called at any time in an unspecified thread
until the receiver is destroyed or the listener is un-associated from it (do we
even allow that?) Calls to received() are guaranteed to be sequential but
they're not guaranteed to be all in the same thread.
That gives us a workable API behind which we can implement a sensible
client-side thread pool initially and add more user-controlled threading options
going forward. I would be inclined to avoid any other threading knobs and dials
(e.g. start() stop() run() type stuff) as that wires assumptions about threading
into the API and restricts what we can do.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: [c++] latest messaging api patch
Posted by Rafael Schloming <ra...@redhat.com>.
Alan Conway wrote:
> On 09/10/2009 10:22 AM, Rafael Schloming wrote:
>> Gordon Sim wrote:
>>> 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.
>>
>> I'm not a fan of this change either. As with acknowledge, reject is
>> really an operation on the state that the session holds about a given
>> message, not an operation on the message itself, so I find it a bit
>> incongruous to have reject on the Message.
>>
>
> Acknowledge acks all messages received by the session to date so its
> clearly a session operation.
>
> Reject is an operation on a specific message. It is implemented as an
> update to the session state, but from the users perspective you reject a
> message. Moreover its only legal to reject a message using the session
> that it arrived on.
>
> Given a choice between:
>
> message.getSession().reject(message)
> message.reject()
>
> the latter seems simpler, more intuitive and less error prone. Why
> complicate the API because of the way the implementation works?
The python API doesn't have a message.getSession() or equivalent. I've
intentionally kept messages as simple value objects so that it's easy to
create them, store them, etc, without having a session around. This of
course forces you to have access to the session when you want to reject
a message, but given that it's a bit more like session.reject(message)
rather than message.getSession().reject(message).
The reason I'm reluctant to create a magic session pointer on the
message is that the client is supposed to be able to arbitrarily process
the message, including things like serializing it to disk, converting it
to an email, waiting for a reply, etc. So by the time the client app
gets around to actually rejecting (or acking the message), I want to
allow for the possibility that it might be a copy (either direct or
indirect), and given that I don't know how that copy is made, I can't
guarantee that the magic session pointer is preserved.
Really I think the most basic form of the API is actually
session.acknowledge/reject(message-id), where there is a defined way of
extracting the message-id from a message, and so long as the message-id
can be extracted, IMHO it shouldn't matter whether the message object is
the same instance or not.
--Rafael
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
RE: [c++] latest messaging api patch
Posted by "Cliff Jansen (Interop Systems Inc)" <v-...@microsoft.com>.
I like the discussion on message content.
Given Alan's recent suggestions, I can easily envisage how an
implementation could optimize the Receiver case and lazily extract the
content from the underlying protocol frames into the form required by
the application, and avoid an intermediate memory copy.
IstreamView(m).read(myMemPtr, m.getContentSize());
i.e., I don't just get the content, I get it where it needed to go.
As a bonus, it can plug right into a decoder, again streaming straight
from the protocol frames.
The reverse direction is not symmetrical. If you insert content into
the message (by assignment or via an ostream), how can the
implementation know at this point what the frame content size will be?
Message m;
// set headers
// insert content... but where is this message going?
mySender.send(m); // frames get created here
I can only see two potential solutions to the problem (being mindful
of the frame set lifecycle issues that were mentioned).
1. provide a way to associate a message (or subclass, or the
MessageContent) with a connection/session on creation, or prior to
inserting content. Something like:
Message m = mySession.CreateMessage();
2. provide an optional pull mechanism, such as assigning an istream to
the content, where the implementation can create the frames directly
at the time of the Sender.send().
The former is more sensible for use with an encoder... a powerful
motivation for this option. The latter compartmentalizes the
performance optimization as an optional-use API and does the right
thing if you send the same message over two different connections.
Plus you could do cutesy things with custom istreams, such as reading
from disk or a database directly into the frame set.
Cliff
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: [c++] latest messaging api patch
Posted by Rafael Schloming <ra...@redhat.com>.
Riggs, Rob wrote:
>> -----Original Message-----
>> From: Gordon Sim [mailto:gsim@redhat.com]
>> Sent: Thursday, September 10, 2009 11:06 AM
>> To: dev@qpid.apache.org
>> Subject: Re: [c++] latest messaging api patch
>>
>> On 09/10/2009 05:35 PM, Alan Conway wrote:
>>> Given a choice between:
>>>
>>> message.getSession().reject(message)
>>> message.reject()
>>>
>>> the latter seems simpler, more intuitive and less error prone. Why
>>> complicate the API because of the way the implementation works?
>> My personal preference is still for Session::reject(const Message&),
>> with the requirement that the message has been received on that session
>> (and not previously acknowledged) made very clear in the documentation.
>
> Alan's suggestion is my preference. APIs should be designed to be difficult, if not impossible, to use incorrectly. The conciseness of Alan's suggested API is also a virtue.
>
> Otherwise, what would be the behavior if the wrong session is asked to reject the message? And what overhead does that entail?
I agree with the principle, but this assumes that it's possible for the
implementation to always maintain a pointer from the message to the
session that you actually want to use, otherwise message.reject() may
end up doing something that you didn't necessarily intend.
With session.reject(message-id) you are always going to get exactly what
you ask for or a clear exception that the id is unrecognized. So I'd
argue that the principle may actually suggest this form rather than
message.reject().
--Rafael
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
RE: [c++] latest messaging api patch
Posted by "Riggs, Rob" <Ro...@epsilon.com>.
> -----Original Message-----
> From: Gordon Sim [mailto:gsim@redhat.com]
> Sent: Thursday, September 10, 2009 11:06 AM
> To: dev@qpid.apache.org
> Subject: Re: [c++] latest messaging api patch
>
> On 09/10/2009 05:35 PM, Alan Conway wrote:
> > Given a choice between:
> >
> > message.getSession().reject(message)
> > message.reject()
> >
> > the latter seems simpler, more intuitive and less error prone. Why
> > complicate the API because of the way the implementation works?
>
> My personal preference is still for Session::reject(const Message&),
> with the requirement that the message has been received on that session
> (and not previously acknowledged) made very clear in the documentation.
Alan's suggestion is my preference. APIs should be designed to be difficult, if not impossible, to use incorrectly. The conciseness of Alan's suggested API is also a virtue.
Otherwise, what would be the behavior if the wrong session is asked to reject the message? And what overhead does that entail?
This e-mail and files transmitted with it are confidential, and are intended solely for the use of the individual or entity to whom this e-mail is addressed. If you are not the intended recipient, or the employee or agent responsible to deliver it to the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you are not one of the named recipient(s) or otherwise have reason to believe that you received this message in error, please immediately notify sender by e-mail, and destroy the original message. Thank You.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: [c++] latest messaging api patch
Posted by Gordon Sim <gs...@redhat.com>.
On 09/10/2009 05:35 PM, Alan Conway wrote:
> Acknowledge acks all messages received by the session to date so its
> clearly a session operation.
>
> Reject is an operation on a specific message. It is implemented as an
> update to the session state, but from the users perspective you reject a
> message. Moreover its only legal to reject a message using the session
> that it arrived on.
>
> Given a choice between:
>
> message.getSession().reject(message)
> message.reject()
>
> the latter seems simpler, more intuitive and less error prone. Why
> complicate the API because of the way the implementation works?
I certainly don't like the former! Most of my dislike for the latter
came from the need to hold a session reference on the message, just to
handle reject (though I concede that does make enforcing the rule on the
session to use easy), so a Message::getSession() would be something I'd
avoid.
FWIW, I envisaged the usage more as:
Message message = session.fetch();
//look at it
session.reject(message);
Of course the message could also be fetched from a specific receiver or
passed to a listener where the association with a session is slightly
less immediate.
My personal preference is still for Session::reject(const Message&),
with the requirement that the message has been received on that session
(and not previously acknowledged) made very clear in the documentation.
However, clearly there are different views and it's not a deal-breaker
for me.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: [c++] latest messaging api patch
Posted by Alan Conway <ac...@redhat.com>.
On 09/10/2009 10:22 AM, Rafael Schloming wrote:
> Gordon Sim wrote:
>> 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.
>
> I'm not a fan of this change either. As with acknowledge, reject is
> really an operation on the state that the session holds about a given
> message, not an operation on the message itself, so I find it a bit
> incongruous to have reject on the Message.
>
Acknowledge acks all messages received by the session to date so its clearly a
session operation.
Reject is an operation on a specific message. It is implemented as an update to
the session state, but from the users perspective you reject a message. Moreover
its only legal to reject a message using the session that it arrived on.
Given a choice between:
message.getSession().reject(message)
message.reject()
the latter seems simpler, more intuitive and less error prone. Why complicate
the API because of the way the implementation works?
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: [c++] latest messaging api patch
Posted by Rafael Schloming <ra...@redhat.com>.
Gordon Sim wrote:
> 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.
I'm not a fan of this change either. As with acknowledge, reject is
really an operation on the state that the session holds about a given
message, not an operation on the message itself, so I find it a bit
incongruous to have reject on the Message.
--Rafael
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: [c++] latest messaging api patch
Posted by Gordon Sim <gs...@redhat.com>.
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:dev-subscribe@qpid.apache.org