Thanks for all the great suggestions, Alan! Comments inline...
Alan Conway wrote:
On 07/31/2009 01:10 PM, Gordon Sim wrote:
Attached is a patch for a higher level, protocol independent messaging
api from c++ as last discussed some months ago. I have not had much time
to spend on this since then and so there is not a huge amount of change
(mainly moving to a better namespace and adjusting for the revised
include directory layout).
Looks good, overall I like the flow of the examples.
We shoudl ensure that Variant::Map and Variant::List provide the full
std::map/sequence API.
At present Variant::Map and Variant::List are just typedefs for
std::map<std::string, Variant> and std::list<Variant> respectively.
map_sender.cpp:
Message message;
message.getContent()["id"] = 987654321;
Why does a message default to having a map content?
It doesn't, but if there is no defined content and you ask for a map,
then you get a map.
message.getContent().asList();
message.getContent()["id"] = 987654321; //ERROR!
Here the second line would fail with an exception as the content cannot
be converted to a map (which is required by the [] operator).
How about:
Map map;
map["id"] = ...
message.setContent(map);
You can do that already (where Map is a Variant::Map).
A map is a valuable data structure that deserves to exist independently
of Message, and a Message
shouldn't be predjudiced towards one particular type of content.
Agree on both points.
In
particular there's no way to
implement getContent()[] if the message contains binary data.
Agreed. At present the [] operator will throw an exception in that case.
It is really just a shortcut for message.asMap()[key], and is perhaps
unnecessary.
Variant::List colours;
We should make it simple & efficient to interop with std:: collections
here, e.g.:
Yes I agree; good suggestion!
std::vector<string> colours;
map["colours"] = Variant::rangeList(colours);
Where rangeList returns a templated wrapper for a pair of iterators that
can be inserted
into a map.
map_receiver.cpp:
print functions: I think all our types including Variant and Map should
have std::ostream op<<, I'd push
the print code into that and use << in the example.
Yes, that would be good.
Also the type
returned by getContent() should have
an ostream op so you can say:
cout << message.getContent();
Agree again.
queue_listener.cpp:
This looks messy:
listener.subscribed(session.subscribe("message_queue", listener));
Informing the listener can be done as part of the implementation of
session.subscribe(), it doesn't
need to be left to the user.
I'll have a think about this.
We need to sort out our threading model in this new API. The critical
thing is to allow many sessions to be served by the same thread or
thread pool pool.
I agree that this is an important use case. I'll go in to this in more
detail shortly.
We also want to integrate that with our own client side poller, and of
course keep the current model for backwards compatibility.
How about providing two alternatives:
Session {
void dispatch(); // Calling thread dispatches session.
// OR
void activate(); // Session is dispatched by qpid's own thread pool.
Does not block.
void deactivate(); // Stop dispatching in qpid thread pool, blocks
till current operation completes.
void wait(); // Wait for activated session to be closed, or the last
subscription to be cancelled.
}
I think that covers the majority of cases. The other case that has
been mentioned is providing a selectable fd to dispatch to qpid from
someone elses select/poll/epoll loop. I'd say that's an addition for
the 1.1 release of the new API, not necessarily for 1.0.
Agree (on both points!). Thanks again for all the comments and suggestions!
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:[email protected]
