I just went over the messaging API on trunk, I scribbled some comments on it
diff attached.
Diff against SVN revision 813100
diff --git a/qpid/cpp/include/qpid/messaging/Address.h b/qpid/cpp/include/qpid/messaging/Address.h
index e66c52f..f9c9d0c 100644
--- a/qpid/cpp/include/qpid/messaging/Address.h
+++ b/qpid/cpp/include/qpid/messaging/Address.h
@@ -37,8 +37,13 @@ namespace messaging {
* this. However this struct allows the type of address to be
* specified allowing more sophisticated treatment if necessary.
*/
+
+// FIXME aconway 2009-09-09: why do we need separate type? Can't it be specified
+// as part of the value, url style?
+
struct Address
{
+ // FIXME aconway 2009-09-09: why public? if public why do we need toStr()
std::string value;
std::string type;
@@ -46,11 +51,15 @@ struct Address
QPID_CLIENT_EXTERN Address(const std::string& address);
QPID_CLIENT_EXTERN Address(const std::string& address, const std::string& type);
QPID_CLIENT_EXTERN operator const std::string&() const;
+ // FIXME aconway 2009-09-09: how does toStr deal with the type?
QPID_CLIENT_EXTERN const std::string& toStr() const;
QPID_CLIENT_EXTERN operator bool() const;
QPID_CLIENT_EXTERN bool operator !() const;
};
+// FIXME aconway 2009-09-09: it feels like Address should be more abstract than just
+// a string but I don't have a good use case to back that up.
+
QPID_CLIENT_EXTERN std::ostream& operator<<(std::ostream& out, const Address& address);
}} // namespace qpid::messaging
diff --git a/qpid/cpp/include/qpid/messaging/Codec.h b/qpid/cpp/include/qpid/messaging/Codec.h
index bacec5c..181a136 100644
--- a/qpid/cpp/include/qpid/messaging/Codec.h
+++ b/qpid/cpp/include/qpid/messaging/Codec.h
@@ -22,12 +22,18 @@
*
*/
#include <string>
+// FIXME aconway 2009-09-09: should be qpid/messaging/ImportExport.h,
+// keep it independent from client API.
#include "qpid/client/ClientImportExport.h"
namespace qpid {
namespace messaging {
class Variant;
+// FIXME aconway 2009-09-09: Don't understand what this class is for.
+// or why it is public API.
+// I think I'd prefer to see Codec::encode/decode(Message&) and have message API be
+// independent of Codec.
/**
*
*/
diff --git a/qpid/cpp/include/qpid/messaging/Filter.h b/qpid/cpp/include/qpid/messaging/Filter.h
index 5cd844c..2e36094 100644
--- a/qpid/cpp/include/qpid/messaging/Filter.h
+++ b/qpid/cpp/include/qpid/messaging/Filter.h
@@ -31,11 +31,17 @@ namespace client {
namespace messaging {
+// FIXME aconway 2009-09-09: it seems to me that a Filter should be an object
+// that filters, not just a set of patterns. How about an abstract Filter
+// base class and a factory to create filters from patterns?
+//
struct Filter
{
+ // FIXME aconway 2009-09-09: why public data?
std::string type;
std::vector<std::string> patterns;
+ // FIXME aconway 2009-09-09: this looks nasty - generalize to 2 rather than N.
QPID_CLIENT_EXTERN Filter(std::string type, std::string pattern);
QPID_CLIENT_EXTERN Filter(std::string type, std::string pattern1, std::string pattern2);
diff --git a/qpid/cpp/include/qpid/messaging/Message.h b/qpid/cpp/include/qpid/messaging/Message.h
index e68d8a1..ac47674 100644
--- a/qpid/cpp/include/qpid/messaging/Message.h
+++ b/qpid/cpp/include/qpid/messaging/Message.h
@@ -75,7 +75,9 @@ class Message
QPID_CLIENT_EXTERN void setContent(const std::string& s);
QPID_CLIENT_EXTERN void setContent(const Variant::Map&);
QPID_CLIENT_EXTERN void setContent(const Variant::List&);
-
+
+ // FIXME aconway 2009-09-09: Message shouldn't know about Codec, other way around.
+ // Codec should have encode/decode(Message&)
QPID_CLIENT_EXTERN void encode(Codec&);
QPID_CLIENT_EXTERN void decode(Codec&);
diff --git a/qpid/cpp/include/qpid/messaging/MessageContent.h b/qpid/cpp/include/qpid/messaging/MessageContent.h
index 7c3a636..28c8c1f 100644
--- a/qpid/cpp/include/qpid/messaging/MessageContent.h
+++ b/qpid/cpp/include/qpid/messaging/MessageContent.h
@@ -29,6 +29,20 @@
namespace qpid {
namespace messaging {
+// FIXME aconway 2009-09-09: I think we need to get away from the assumption that
+// a message must be represented as a contiguous set of bytes and must be copied.
+// to/from user data.
+//
+// If we want to support efficient << style streaming of data into a message then we need
+// to allow a message to contain multiple buffers rather than resizing on every <<.
+//
+// If we want to support no-copy of user data for large messages then we should allow
+// user buffers to be added to the message.
+//
+// I'll try to have a think about this and draft up something for you to consider.
+// We may be able to use the patterns of std::iostream.
+//
+
/**
*
*/
@@ -37,6 +51,8 @@ class MessageContent
public:
QPID_CLIENT_EXTERN virtual ~MessageContent() {}
+ // FIXME aconway 2009-09-09: Should be a PIMPL, not an abstract base class.
+
virtual const std::string& asString() const = 0;
virtual std::string& asString() = 0;
@@ -60,6 +76,11 @@ class MessageContent
//operator<< for variety of types... (is this a good idea?)
+ // FIXME aconway 2009-09-09: I would say no. We don't want to
+ // tie message to one particular encoding. This kind of
+ // convenience can be added externally it does not need to
+ // be built in.
+ //
virtual MessageContent& operator<<(const std::string&) = 0;
virtual MessageContent& operator<<(const char*) = 0;
virtual MessageContent& operator<<(bool) = 0;
diff --git a/qpid/cpp/include/qpid/messaging/Receiver.h b/qpid/cpp/include/qpid/messaging/Receiver.h
index e51e109..c55ec4e 100644
--- a/qpid/cpp/include/qpid/messaging/Receiver.h
+++ b/qpid/cpp/include/qpid/messaging/Receiver.h
@@ -81,7 +81,12 @@ class Receiver : public qpid::client::Handle<ReceiverImpl>
* serving before throwing an exception.
*/
QPID_CLIENT_EXTERN Message fetch(qpid::sys::Duration timeout=qpid::sys::TIME_INFINITE);
-
+
+ // FIXME aconway 2009-09-09: what do start/stop mean for a pull receiver?
+ // The message flow as far as the user is concerned is controlled by get/fetch.
+ // I think we should be able to drop these.
+ //
+
/**
* Enables the message flow for this receiver
*/
@@ -96,6 +101,8 @@ class Receiver : public qpid::client::Handle<ReceiverImpl>
* many incoming messages can be held in the receiver before being
* requested by a client via fetch() (or pushed to a listener).
*/
+ // FIXME aconway 2009-09-09: ?? Is this setting a flow control
+ // window? What happens if capacity is exceeded and there's no listener?
QPID_CLIENT_EXTERN void setCapacity(uint32_t);
/**
@@ -106,10 +113,17 @@ class Receiver : public qpid::client::Handle<ReceiverImpl>
/**
* Set a message listener for receiving messages asynchronously.
*/
+ // FIXME aconway 2009-09-09: how does this interact with get/fetch?
+ // seems like they are orthogonal and don't belong on the same class.
+ // Maybe split this into a base class and a PullReceiver and PushReciever?
+ // I think start/stop above belong on the PushReceiver not the base class.
QPID_CLIENT_EXTERN void setListener(MessageListener* listener);
private:
friend class qpid::client::PrivateImplRef<Receiver>;
};
+
+// FIXME aconway 2009-09-09: where do we set flow control?
+
}} // namespace qpid::messaging
#endif /*!QPID_MESSAGING_RECEIVER_H*/
diff --git a/qpid/cpp/include/qpid/messaging/Session.h b/qpid/cpp/include/qpid/messaging/Session.h
index 1d88882..555069e 100644
--- a/qpid/cpp/include/qpid/messaging/Session.h
+++ b/qpid/cpp/include/qpid/messaging/Session.h
@@ -70,16 +70,25 @@ class Session : public qpid::client::Handle<SessionImpl>
* Rejects the specified message. This will prevent the message
* being redelivered.
*/
+ // FIXME aconway 2009-09-09:
+ // Message::reject seems more natural and is less error prone as we
+ // can internally manage the link from a Message to the Session it arrived on.
QPID_CLIENT_EXTERN void reject(Message&);
QPID_CLIENT_EXTERN void sync();
QPID_CLIENT_EXTERN void flush();
+ // FIXME aconway 2009-09-09: why do we need Session::fetch? How does it
+ // interact with fetch on receivers for the session?
QPID_CLIENT_EXTERN bool fetch(Message& message, qpid::sys::Duration timeout=qpid::sys::TIME_INFINITE);
QPID_CLIENT_EXTERN Message fetch(qpid::sys::Duration timeout=qpid::sys::TIME_INFINITE);
QPID_CLIENT_EXTERN bool dispatch(qpid::sys::Duration timeout=qpid::sys::TIME_INFINITE);
+ // FIXME aconway 2009-09-09: The versions below that takes a string address
+ // are not needed. Address already defines a conversion from string.
+
QPID_CLIENT_EXTERN Sender createSender(const Address& address, const VariantMap& options = VariantMap());
+
QPID_CLIENT_EXTERN Sender createSender(const std::string& address, const VariantMap& options = VariantMap());
QPID_CLIENT_EXTERN Receiver createReceiver(const Address& address, const VariantMap& options = VariantMap());
@@ -89,8 +98,10 @@ class Session : public qpid::client::Handle<SessionImpl>
QPID_CLIENT_EXTERN Address createTempQueue(const std::string& baseName = std::string());
+ // FIXME aconway 2009-09-09: what do these mean? And why are they using the evil void*?
QPID_CLIENT_EXTERN void* getLastConfirmedSent();
QPID_CLIENT_EXTERN void* getLastConfirmedAcknowledged();
+
private:
friend class qpid::client::PrivateImplRef<Session>;
};
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:[email protected]
