astitcher commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r773465245
##########
File path: cpp/include/proton/target_options.hpp
##########
@@ -27,6 +27,7 @@
#include "./duration.hpp"
#include "./target.hpp"
+#include <map>
#include <string>
Review comment:
Same comment here as above re ```#include <vector>```
##########
File path: cpp/include/proton/terminus.hpp
##########
@@ -103,6 +104,10 @@ class terminus {
/// Extension capabilities that are supported/requested
PN_CPP_EXTERN std::vector<symbol> capabilities() const;
+ /// Obtain the AMQP dynamic node properties for the
+ /// terminus as a standard map.
+ PN_CPP_EXTERN std::map<symbol, value> dynamic_properties();
Review comment:
I'm pretty sure this should also be a ```const``` method. In general
getters should be ```const``` as they don't change the object itself.
##########
File path: cpp/src/terminus.cpp
##########
@@ -61,4 +61,12 @@ std::vector<symbol> terminus::capabilities() const {
return caps.empty() ? std::vector<symbol>() : caps.get<std::vector<symbol>
>();
}
+std::map<symbol, value> terminus::dynamic_properties() {
Review comment:
As per my comment in the header file this method should be ```const```
##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,114 @@
#include <proton/sender_options.hpp>
#include <proton/receiver_options.hpp>
#include <proton/container.hpp>
+#include <proton/connection.hpp>
+#include <proton/connection_options.hpp>
+#include <proton/listen_handler.hpp>
+#include <proton/listener.hpp>
+#include <proton/messaging_handler.hpp>
+#include <proton/types.hpp>
+#include <proton/message.hpp>
+#include <proton/target_options.hpp>
+#include <proton/source_options.hpp>
+#include <proton/delivery.hpp>
#include <iostream>
+#include <map>
+#include <condition_variable>
+#include <mutex>
+#include <thread>
+
+namespace {
+std::mutex m;
+std::condition_variable cv;
+bool listener_ready = false;
+int listener_port;
+const std::string DNP_SYMBOL = "DNP_SYMBOL";
+const std::string DNP_VALUE = "DNP_VALUE";
+} // namespace
Review comment:
If you are trying to replicate a realistic scenario as @gemmellr is
suggesting maybe you want to use real property values as defined in the section
3.5.9 of the AMQP standard?
If I read the standard correctly the 2 defined properties are the symbols
"lifetime-policy" and "supported-dist-modes":
The allowed values of lifetime policy seem to be described lists with values
'@delete-on-closed(43)[]', '@delete-on-nolinks(44)[]',
'@delete-on-no-messages{45)[]', '@delete-on-no-links-or-messages(46)[]' - these
all have specific meanings as to when the creator of the dynamic node should
delete the node. As a side note it seems unusual but these property values seem
to be described lists which should always be empty as there is no useful thing
to put in them.
The allowed values for 'supported-dist-modes' are the symbols 'move' and
'copy' and the list '['move', copy'] (or in the other order!). Or even single
element lists with just symbols 'copy' or 'move' I think.
Of course we have no code in the C++ binding that actually cares what the
values are so this doesn't matter a whole lot, but it might be nice to generate
realistic dynamic property values.
##########
File path: cpp/src/node_options.cpp
##########
@@ -19,15 +19,17 @@
*
*/
-#include "proton/codec/vector.hpp"
Review comment:
Not sure why this has been removed. It doesn't seem related to your
changes. If it's not needed it would be better (and less distracting) to remove
it in a separate change (it could be NO-JIRA if you're really sure it's not
needed - effectively that is what this change is!)
##########
File path: cpp/include/proton/source_options.hpp
##########
@@ -27,6 +27,7 @@
#include "./duration.hpp"
#include "./source.hpp"
+#include <map>
#include <string>
Review comment:
[@DreamPearl Not something you changed] I'm wondering why this header
doesn't ```#include <vector>``` as it clearly uses ```std::vector`` in the
capabilities API. I think this would be a separate change anyway!
##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,114 @@
#include <proton/sender_options.hpp>
#include <proton/receiver_options.hpp>
#include <proton/container.hpp>
+#include <proton/connection.hpp>
+#include <proton/connection_options.hpp>
+#include <proton/listen_handler.hpp>
+#include <proton/listener.hpp>
+#include <proton/messaging_handler.hpp>
+#include <proton/types.hpp>
+#include <proton/message.hpp>
+#include <proton/target_options.hpp>
+#include <proton/source_options.hpp>
+#include <proton/delivery.hpp>
#include <iostream>
+#include <map>
+#include <condition_variable>
+#include <mutex>
+#include <thread>
+
+namespace {
+std::mutex m;
+std::condition_variable cv;
+bool listener_ready = false;
+int listener_port;
+} // namespace
+
+class test_recv : public proton::messaging_handler {
+ private:
+ class listener_ready_handler : public proton::listen_handler {
+ void on_open(proton::listener &l) override {
+ {
+ std::lock_guard<std::mutex> lk(m);
+ listener_port = l.port();
+ listener_ready = true;
+ }
+ cv.notify_one();
+ }
+ };
+
+ std::string url;
+ proton::listener listener;
+ listener_ready_handler listen_handler;
+
+ public:
+ test_recv(const std::string &s) : url(s) {}
+
+ void on_container_start(proton::container &c) override {
+ listener = c.listen(url, listen_handler);
+ }
+
+ void on_message(proton::delivery &d, proton::message &msg) override {
+ proton::symbol sym = "symbol";
+ proton::value val = "value";
+ std::map<proton::symbol, proton::value> props =
d.receiver().target().dynamic_node_properties();
+
+ ASSERT(!props.empty());
+ for(auto it=props.begin(); it!=props.end(); it++) {
+ ASSERT_EQUAL(sym, it->first);
+ ASSERT_EQUAL(val, it->second);
+ }
+ d.receiver().close();
+ d.connection().close();
+ listener.stop();
+ }
+};
+
+class test_send : public proton::messaging_handler {
+ private:
+ std::string url;
+ proton::sender sender;
+
+ public:
+ test_send(const std::string &s) : url(s) {}
+
+ void on_container_start(proton::container &c) override {
+ proton::target_options opts;
+ std::map<proton::symbol,proton::value> m({{proton::symbol("symbol"),
proton::value("value")}});
+ opts.dynamic_node_properties(m);
+ sender = c.open_sender(url, proton::sender_options().target(opts));
+ }
+
+ void on_sendable(proton::sender &s) override {
+ proton::message msg;
+ msg.body("message");
+ proton::tracker t = s.send(msg);
+ s.connection().close();
+ }
+};
+
+int test_dynamic_node_properties() {
Review comment:
Although @gemmellr is correct in his analysis of what is going on in a
real AMQP messaging interaction, this is probably not all that important as the
C++ binding (at least currently) has no logic at either sender or receiver to
do anything special for dynamic nodes - it just passes the info on to the
application which is expected to validate and act on that information.
So from the point of view of this test it is only important to check that
the properties get correctly carried in both directions. Unless the tester
itself generates the requested nodes the API is not going to so there really
isn't anything to validate in the test.
In a future change to the C++ binding it would definitely make sense to
validate that you can't set address and set dynamic if you are sender/target or
receiver/source. And also that if dynamic node properties are set the dynamic
must also be set for the node, but that isn't done by the binding (or proton-c)
currently so testing for it makes no sense. Producing realistic protocol
interactions makes some sense but only to make the test "more realistic".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
