You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2021/12/21 22:47:46 UTC
[GitHub] [qpid-proton] astitcher commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
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: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org