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/09 17:22:46 UTC
[GitHub] [qpid-proton] DreamPearl opened a new pull request #346: [WIP] PROTON-2308: Add support for setting Dynamic Node Properties
DreamPearl opened a new pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346
[PROTON-2308](https://issues.apache.org/jira/browse/PROTON-2308)
--
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
[GitHub] [qpid-proton] gemmellr commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r773070502
##########
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:
That bit sounds/looks good. There are peripheral issues as mentioned before that should also be addressed.
The trace log shows the test still uses an illegal target definition in the clients sender attach, where it sets dynamic-node-properties, but doesnt set dynamic=true, and sets an address "test" for the target (which the server naively copies and so reflects back). Thats then compounded when the test/client expects a 'response' attach with dynamic=true, something which shouldn't occur in this scenario.
Right now it verifies there are [different] dynamic node properties on the the servers 'response' attach, and the dynamic flag is set on the target, but not that there is a generated address. It would be good for the test to verify the whole of the 'dynamic' interation, i.e that the client 'requests' dynamic=true and doesnt set an address, and that the client can pick up the address value set by the server once the link opens (since the dynamic node would be useless without an address).
I think it would be better if the server did its work in the 'on_receiver_open' (or per next comment, sender open) method rather than using the options in the container init, since you wont/cant generally use the options in that situation as a server as not everything is dynamic, and each dynamic node needs its own address etc. When the link attaches the server could verify it got a proper/expected request for a dynamic node, and respond setting the appropriate details on the target directly (or probably rather the source, per next comment), rather than using the options and also might as well open the link itself and not bothering with calling messaging_handler::on_receiver_open(r);).
Main other comment would be that this is still only trying to test a 'dynamic sender' from the clients perspective, when almost noone will ever do that, so it should either use a 'dynamic receiver' or test both.
--
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
[GitHub] [qpid-proton] DreamPearl commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
DreamPearl commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r771315464
##########
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:
Well, I am also wondering if the other direction needs to be tested as well or not. I gave it a try (set the properties on the receiver and try to read them from the sender in the same test).
- If I am setting dynamic node properties on the receiver like this `c.receiver_options(proton::receiver_options().source(opts));` and reading it from the sender then the test is getting terminated. And I can not see them getting set in the `attach frame`, plus the SOURCE in the third and fourth attach frame is NULL, which might be the reason for termination.
- But if I am setting it like this`c.receiver_options(proton::receiver_options().target(opts));` , then all is good.
The question is which one is correct or none of them is correct.
Meanwhile, I am looking for resources to get the idea.
Below are the attach frames:
```
[0x1fef6d0]: AMQP:FRAME:0 -> @attach(18) [name="3d0a3e34-a9fd-42b8-9cfd-0b1bbcc9c910", handle=0x0,
role=false, snd-settle-mode=0x2, rcv-settle-mode=0x0, source=@source(40) [address=null, durable=0x0, expiry-
policy=null, timeout=0x0, dynamic=false, dynamic-node-properties=null, distribution-mode=null, filter=null, default-
outcome=null, outcomes=null, capabilities=null], target=@target(41) [address="test", durable=0x0, expiry-policy=null,
timeout=0x0, dynamic=false, dynamic-node-properties={:symbol="value"}, capabilities=null], unsettled=null, incomplete-
unsettled=null, initial-delivery-count=0x0, max-message-size=0x0, offered-capabilities=null, desired-capabilities=null,
properties=null]
```
```
[0x7f7ffc002de0]: AMQP:FRAME:0 <- @attach(18) [name="3d0a3e34-a9fd-42b8-9cfd-0b1bbcc9c910", handle=0x0,
role=false, snd-settle-mode=0x2, rcv-settle-mode=0x0, source=@source(40) [address=null, durable=0x0, expiry-
policy=null, timeout=0x0, dynamic=false, dynamic-node-properties=null, distribution-mode=null, filter=null, default-
outcome=null, outcomes=null, capabilities=null], target=@target(41) [address="test", durable=0x0, expiry-policy=null,
timeout=0x0, dynamic=false, dynamic-node-properties={:symbol="value"}, capabilities=null], unsettled=null, incomplete-
unsettled=null, initial-delivery-count=0x0, max-message-size=0x0, offered-capabilities=null, desired-capabilities=null,
properties=null]
```
```
[0x7f7ffc002de0]: AMQP:FRAME:0 -> @attach(18) [name="3d0a3e34-a9fd-42b8-9cfd-0b1bbcc9c910", handle=0x0,
role=true, snd-settle-mode=0x2, rcv-settle-mode=0x0, source=null, target=@target(41) [address="test", durable=0x0,
expiry-policy=null, timeout=0x0, dynamic=false, dynamic-node-properties={:symbol2="value2"}, capabilities=null],
unsettled=null, incomplete-unsettled=null, initial-delivery-count=0x0, max-message-size=0x0, offered-capabilities=null,
desired-capabilities=null, properties=null]
```
```
[0x1fef6d0]: AMQP:FRAME:0 <- @attach(18) [name="3d0a3e34-a9fd-42b8-9cfd-0b1bbcc9c910", handle=0x0, role=true,
snd-settle-mode=0x2, rcv-settle-mode=0x0, source=null, target=@target(41) [address="test", durable=0x0, expiry-
policy=null, timeout=0x0, dynamic=false, dynamic-node-properties={:symbol2="value2"}, capabilities=null],
unsettled=null, incomplete-unsettled=null, initial-delivery-count=0x0, max-message-size=0x0, offered-capabilities=null,
desired-capabilities=null, properties=null]
```
[Complete logs](https://gist.github.com/DreamPearl/f35d3431a28c0432ad04352c4b0bffa5)
--
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
[GitHub] [qpid-proton] gemmellr commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r773945393
##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,121 @@
#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 DYNAMIC_ADDRESS = "test_dynamic_address";
+} // 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_receiver_open(proton::receiver& r) override {
+ std::map<proton::symbol,proton::value> m_sender({{proton::symbol("supported-dist-modes"), proton::symbol("move")}});
+ std::map<proton::symbol,proton::value> props = r.target().dynamic_properties();
+
+ ASSERT(r.target().dynamic());
+ ASSERT(r.target().address().empty());
Review comment:
Is this checking for an empty string or for null (or either)? I would expect the address not to be present at all, i.e null. The empty string is another thing entirely.
##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,121 @@
#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 DYNAMIC_ADDRESS = "test_dynamic_address";
+} // 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_receiver_open(proton::receiver& r) override {
+ std::map<proton::symbol,proton::value> m_sender({{proton::symbol("supported-dist-modes"), proton::symbol("move")}});
+ std::map<proton::symbol,proton::value> props = r.target().dynamic_properties();
+
+ ASSERT(r.target().dynamic());
+ ASSERT(r.target().address().empty());
+ ASSERT_EQUAL(m_sender, props);
+
+ proton::target_options opts;
+ std::map<proton::symbol, proton::value> m({{proton::symbol("supported-dist-modes"), proton::symbol("copy")}});
+ opts.address(DYNAMIC_ADDRESS);
+ opts.dynamic_properties(m);
+ if (r.uninitialized()) {
+ r.open(proton::receiver_options().target(opts));
+ }
+ listener.stop();
+ }
+};
+
+class test_send : public proton::messaging_handler {
+ private:
+ std::string url;
+
+ 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("supported-dist-modes"), proton::symbol("move")}});
+ opts.dynamic(true);
+ opts.dynamic_properties(m);
+ c.open_sender(url, proton::sender_options().target(opts));
Review comment:
This continues to use a sender from the clients perspective. If we only test one of them then I think it should be initiating a receiver as thats what people will mostly tend to do, whereas this is testing a dynamic sender which is something that few will ever do.
##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,121 @@
#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 DYNAMIC_ADDRESS = "test_dynamic_address";
+} // 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_receiver_open(proton::receiver& r) override {
+ std::map<proton::symbol,proton::value> m_sender({{proton::symbol("supported-dist-modes"), proton::symbol("move")}});
+ std::map<proton::symbol,proton::value> props = r.target().dynamic_properties();
+
+ ASSERT(r.target().dynamic());
+ ASSERT(r.target().address().empty());
+ ASSERT_EQUAL(m_sender, props);
+
+ proton::target_options opts;
+ std::map<proton::symbol, proton::value> m({{proton::symbol("supported-dist-modes"), proton::symbol("copy")}});
+ opts.address(DYNAMIC_ADDRESS);
+ opts.dynamic_properties(m);
+ if (r.uninitialized()) {
Review comment:
Is this checking the link isnt open/closed already so it doesnt try to open it again? If so I dont really understand the need for this in this test, as the server peer should never initiate the link. If the check is needed I would probably expect an else that failed the test somehow.
--
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
[GitHub] [qpid-proton] DreamPearl commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
DreamPearl commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r773948252
##########
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 actually removed it temporarily and forgot to add it back. :(
--
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
[GitHub] [qpid-proton] gemmellr commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r774059843
##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,121 @@
#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 DYNAMIC_ADDRESS = "test_dynamic_address";
+} // 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_receiver_open(proton::receiver& r) override {
+ std::map<proton::symbol,proton::value> m_sender({{proton::symbol("supported-dist-modes"), proton::symbol("move")}});
+ std::map<proton::symbol,proton::value> props = r.target().dynamic_properties();
+
+ ASSERT(r.target().dynamic());
+ ASSERT(r.target().address().empty());
Review comment:
If the API is intended to return an empty string then I think that its ok if that is all the test verifies. I dont know about adding use of the C function in the C++ tests, not my area of expertise at all, though if it was easy and not frowned upon then I personally would consider verifying the value fully to be worthwhile. But if isnt simple or is frowned upon to do such things then thats fair enough.
I wasnt suggesting you add C tests (though I do think they _should_ already exist)
--
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
[GitHub] [qpid-proton] gemmellr commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r773774519
##########
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:
The supported-dist-modes would be the simplest real noe property (as anything legal is allwoed I didnt see much need to use specific values, but it would be a representative test). The supported-dist-modes value is noted as having a 'multiple' definition, which means it can takes a single symbol or an array (not list) of symbols, with null or an empty array both meaning not-specified (per section 1.4 Composite Type Representation in the spec).
--
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
[GitHub] [qpid-proton] gemmellr commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r773774519
##########
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:
The supported-dist-modes would be the simplest real node property (as anything legal is allowed I didnt see much need to use specific values, but it would be a representative test). The supported-dist-modes value is noted as having a 'multiple' definition, which means it can takes a single symbol or an array (not list) of symbols, with null or an empty array both meaning not-specified (per section 1.4 Composite Type Representation in the spec).
--
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
[GitHub] [qpid-proton] gemmellr commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r773070502
##########
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:
That bit sounds/looks good. There are peripheral issues as mentioned before that should also be addressed.
The trace log shows the test still uses an illegal target definition in the clients sender attach, where it sets dynamic-node-properties, but doesnt set dynamic=true, and sets an address "test" for the target (which the server naively copies and so reflects back). Thats then compounded when the test/client expects a 'response' attach with dynamic=true, something which shouldn't occur in this scenario.
Right now it verifies there are [different] dynamic node properties on the the servers 'response' attach, and the dynamic flag is set on the target, but not that there is a generated address. It would be good for the test to verify the whole of the 'dynamic' interation, i.e that the client 'requests' dynamic=true and doesnt set an address, and that the client can pick up the address value set by the server once the link opens (since the dynamic node would be useless without an address).
I think it would be better if the server did its work in the 'on_receiver_open' (or per next comment, sender open) method rather than using the options in the container init, since you wont/cant generally use the options in that situation as a server as not everything is dynamic, and each dynamic node needs its own address etc. When the link attaches the server could verify it got a proper/expected request for a dynamic node, and respond setting the appropriate details on the target directly (or probably rather the source, per next comment) rather than using the options (EDIT: or it seems you can supply link-specific options when calling open, perhaps that instead) and also might as well open the link itself and not bothering with calling messaging_handler::on_receiver_open(r);).
Main other comment would be that this is still only trying to test a 'dynamic sender' from the clients perspective, when almost noone will ever do that, so it should either use a 'dynamic receiver' or test both.
--
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
[GitHub] [qpid-proton] codecov-commenter edited a comment on pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#issuecomment-995011027
# [Codecov](https://codecov.io/gh/apache/qpid-proton/pull/346?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#346](https://codecov.io/gh/apache/qpid-proton/pull/346?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b020661) into [main](https://codecov.io/gh/apache/qpid-proton/commit/996b9b114fdb4682c8114ad700705446ff3a24fd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (996b9b1) will **increase** coverage by `20.12%`.
> The diff coverage is `n/a`.
> :exclamation: Current head b020661 differs from pull request most recent head 08c96c7. Consider uploading reports for the commit 08c96c7 to get more accurate results
[](https://codecov.io/gh/apache/qpid-proton/pull/346?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## main #346 +/- ##
===========================================
+ Coverage 68.22% 88.34% +20.12%
===========================================
Files 356 47 -309
Lines 73060 2394 -70666
===========================================
- Hits 49843 2115 -47728
+ Misses 23217 279 -22938
```
| [Impacted Files](https://codecov.io/gh/apache/qpid-proton/pull/346?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [c/src/proactor/epoll.c](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy9zcmMvcHJvYWN0b3IvZXBvbGwuYw==) | | |
| [cpp/include/proton/terminus.hpp](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL2luY2x1ZGUvcHJvdG9uL3Rlcm1pbnVzLmhwcA==) | | |
| [cpp/src/link\_test.cpp](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL3NyYy9saW5rX3Rlc3QuY3Bw) | | |
| [cpp/src/node\_options.cpp](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL3NyYy9ub2RlX29wdGlvbnMuY3Bw) | | |
| [cpp/src/terminus.cpp](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL3NyYy90ZXJtaW51cy5jcHA=) | | |
| [python/proton/\_\_init\_\_.py](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHl0aG9uL3Byb3Rvbi9fX2luaXRfXy5weQ==) | | |
| [c/tests/fuzz/fuzz-message-decode.c](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy90ZXN0cy9mdXp6L2Z1enotbWVzc2FnZS1kZWNvZGUuYw==) | | |
| [c/tests/ssl\_proactor\_test.cpp](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy90ZXN0cy9zc2xfcHJvYWN0b3JfdGVzdC5jcHA=) | | |
| [c/src/core/util.c](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy9zcmMvY29yZS91dGlsLmM=) | | |
| [c/tests/data\_test.cpp](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy90ZXN0cy9kYXRhX3Rlc3QuY3Bw) | | |
| ... and [299 more](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-proton/pull/346?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/qpid-proton/pull/346?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [996b9b1...08c96c7](https://codecov.io/gh/apache/qpid-proton/pull/346?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [qpid-proton] DreamPearl commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
DreamPearl commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r773976052
##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,121 @@
#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 DYNAMIC_ADDRESS = "test_dynamic_address";
+} // 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_receiver_open(proton::receiver& r) override {
+ std::map<proton::symbol,proton::value> m_sender({{proton::symbol("supported-dist-modes"), proton::symbol("move")}});
+ std::map<proton::symbol,proton::value> props = r.target().dynamic_properties();
+
+ ASSERT(r.target().dynamic());
+ ASSERT(r.target().address().empty());
Review comment:
Internally the address is NULL but while retrieving address() as std::string, the NULL will get converted to empty string as per [target::address()](https://github.com/apache/qpid-proton/blob/main/cpp/src/target.cpp#L41)
Trace: https://gist.github.com/DreamPearl/229a5878a3935e43b6a0dfca9490226a
--
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
[GitHub] [qpid-proton] jiridanek commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r770823901
##########
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 a reference to the AMQP dynamic node properties for the
+ /// terminus as a standard map.
+ PN_CPP_EXTERN std::map<symbol, value> dynamic_node_properties();
Review comment:
Comment says reference, but function returns a copy?
--
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
[GitHub] [qpid-proton] astitcher commented on pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
astitcher commented on pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#issuecomment-999598356
> > 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.
>
> I very much disagree. The test should verify functionality actually works the way it needs to be used, and thus help ensure someone doesn't go breaking it later. A test of 'dynamic node [properties]' usage that doesnt actually use the handling/options for dynamic nodes in the required way is simply not a good test. It means it has never been verified to fully work in the situation it needs to, and its never being fully verified it isnt broken later. Assuming things work a certain way is how so many bugs come up when it later turns out it doesnt.
I'm not sure we're actually disagreeing very much here! I agreee it makes the most sense to test the API in the way that it would be actually used - my major point is simply that the binding code has no magic in it to ensure correct AMQP semantics and so this cannot be tested here. A minor point we do disagree on is that I think testing that the values get transferred correctly is the most important point and I think @gemmellr is saying that is not much use unless it actually tests what might be used - I don't think this disagreement matters much! Doing it the @gemmellr way satisfies me too!
>
> If there is a test of the 'no node properties' dynamic usage elsewhere then that would at least mean the 2 bits were somewhat tested in isolation even if the node-props bit was still not actually in full (which would still be poor but not quite as much). I might expect that other testing to be in this same test class, but it doesnt appear so. Is that bit actually tested right now anywhere?
>
Yes it would be really good to have that test here too! But currently we have no C++ test for this at all.
--
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
[GitHub] [qpid-proton] gemmellr commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r774020425
##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,121 @@
#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 DYNAMIC_ADDRESS = "test_dynamic_address";
+} // 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_receiver_open(proton::receiver& r) override {
+ std::map<proton::symbol,proton::value> m_sender({{proton::symbol("supported-dist-modes"), proton::symbol("move")}});
+ std::map<proton::symbol,proton::value> props = r.target().dynamic_properties();
+
+ ASSERT(r.target().dynamic());
+ ASSERT(r.target().address().empty());
Review comment:
Ah I see, I guess that will need to do if that is the API unfortunately.
(Another good example where a test peer that validated wire behaviour without the API would be useful)
--
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
[GitHub] [qpid-proton] codecov-commenter commented on pull request #346: [WIP] PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#issuecomment-995011027
# [Codecov](https://codecov.io/gh/apache/qpid-proton/pull/346?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#346](https://codecov.io/gh/apache/qpid-proton/pull/346?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1cfdda3) into [main](https://codecov.io/gh/apache/qpid-proton/commit/f34c8a251da84a6c6f9a5ba03e746e8ee60ee6a3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f34c8a2) will **increase** coverage by `19.69%`.
> The diff coverage is `n/a`.
> :exclamation: Current head 1cfdda3 differs from pull request most recent head c3b9886. Consider uploading reports for the commit c3b9886 to get more accurate results
[](https://codecov.io/gh/apache/qpid-proton/pull/346?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## main #346 +/- ##
===========================================
+ Coverage 68.65% 88.34% +19.69%
===========================================
Files 351 47 -304
Lines 70788 2394 -68394
===========================================
- Hits 48596 2115 -46481
+ Misses 22192 279 -21913
```
| [Impacted Files](https://codecov.io/gh/apache/qpid-proton/pull/346?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [c/examples/direct.c](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy9leGFtcGxlcy9kaXJlY3QuYw==) | | |
| [c/examples/send-abort.c](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy9leGFtcGxlcy9zZW5kLWFib3J0LmM=) | | |
| [c/examples/send-ssl.c](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy9leGFtcGxlcy9zZW5kLXNzbC5j) | | |
| [c/examples/send.c](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy9leGFtcGxlcy9zZW5kLmM=) | | |
| [c/src/core/buffer.c](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy9zcmMvY29yZS9idWZmZXIuYw==) | | |
| [c/src/core/codec.c](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy9zcmMvY29yZS9jb2RlYy5j) | | |
| [c/src/core/connection\_driver.c](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy9zcmMvY29yZS9jb25uZWN0aW9uX2RyaXZlci5j) | | |
| [c/src/core/data.h](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy9zcmMvY29yZS9kYXRhLmg=) | | |
| [c/src/core/dispatcher.c](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy9zcmMvY29yZS9kaXNwYXRjaGVyLmM=) | | |
| [c/src/core/encoder.c](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy9zcmMvY29yZS9lbmNvZGVyLmM=) | | |
| ... and [294 more](https://codecov.io/gh/apache/qpid-proton/pull/346/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-proton/pull/346?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/qpid-proton/pull/346?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [7c593ce...c3b9886](https://codecov.io/gh/apache/qpid-proton/pull/346?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [qpid-proton] astitcher commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [qpid-proton] astitcher commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
astitcher commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r773917402
##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,121 @@
#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 DYNAMIC_ADDRESS = "test_dynamic_address";
+} // 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_receiver_open(proton::receiver& r) override {
+ std::map<proton::symbol,proton::value> m_sender({{proton::symbol("supported-dist-modes"), proton::value("move")}});
+ std::map<proton::symbol,proton::value> props = r.target().dynamic_properties();
+
+ ASSERT(r.target().dynamic());
+ ASSERT(r.target().address().empty());
+ ASSERT_EQUAL(m_sender, props);
+
+ proton::target_options opts;
+ std::map<proton::symbol, proton::value> m({{proton::symbol("supported-dist-modes"), proton::value("copy")}});
+ opts.address(DYNAMIC_ADDRESS);
+ opts.dynamic_properties(m);
+ if (r.uninitialized()) {
+ r.open(proton::receiver_options().target(opts));
+ }
+ listener.stop();
+ }
+};
+
+class test_send : public proton::messaging_handler {
+ private:
+ std::string url;
+
+ 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("supported-dist-modes"), proton::value("move")}});
Review comment:
As above - maybe it would be better to have constants for move and copy symbols that can be used in this code to avoid repeating ```proton::value(proton::symbol("move"))``` etc. actually perhaps ```proton::symbol("move")``` may be enough as I think it should automatically convert to ```proton::value```
--
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
[GitHub] [qpid-proton] gemmellr commented on pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#issuecomment-999480504
> 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.
I very much disagree. The test should verify functionality actually works the way it needs to be used, and thus help ensure someone doesn't go breaking it later. A test of 'dynamic node [properties]' usage that doesnt actually use the handling/options for dynamic nodes in the required way is simply not a good test. It means it has never been verified to fully work in the situation it needs to, and its never being fully verified it isnt broken later. Assuming things work a certain way is how so many bugs come up when it later turns out it doesnt.
If there is a test of the 'no node properties' dynamic usage elsewhere then that would at least mean the 2 bits were somewhat tested in isolation even if the node-props bit was still not actually in full (which would still be poor but not quite as much). I might expect that other testing to be in this same test class, but it doesnt appear so. Is that bit actually tested right now anywhere?
>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".
Youd simply check the request, set an address value to pass back, mark it dynamic, and then verify the receiver got that address and flag. That would in fact verify that the client options to set dynamic work as expected, that the client is able to read back the provided dynamic address as it needs to be able to, and inspect the dynamic flag was set appropriately. Thats not 'nothing to validate', and is not about just being 'more realistic', but about actually testing what needs to work to use this functionality.
I didnt suggest adding negative tests thats you cant set the things in the illegal way you can currently, only that the test should cover testing it actually does the things it should do when used the way it should be, which appears untested.
--
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
[GitHub] [qpid-proton] jiridanek commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r770834050
##########
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:
This test sets properties on a sender and then reads them out from the receiver's target. I am wondering if the other direction is also needed to be tested: set properties on receiver and read them from sender. Possibly not.
--
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
[GitHub] [qpid-proton] asfgit closed pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346
--
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
[GitHub] [qpid-proton] DreamPearl commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
DreamPearl commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r773943847
##########
File path: cpp/include/proton/source_options.hpp
##########
@@ -27,6 +27,7 @@
#include "./duration.hpp"
#include "./source.hpp"
+#include <map>
#include <string>
Review comment:
Got it. I will raise a small PR for this.
--
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
[GitHub] [qpid-proton] astitcher commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
astitcher commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r773912008
##########
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:
Oh, yes - I forgot that multiple means array not list! A painful (but semantically correct!) decision being the only place in the standard that arrays are required!
--
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
[GitHub] [qpid-proton] astitcher commented on pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
astitcher commented on pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#issuecomment-999599982
As a side note making properties that represent the documented AMQP possibilities is not actually all that easy as I said in my previous comment. We maybe need something predefined to allow the auto delete behaviours to be set more conveniently.
--
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
[GitHub] [qpid-proton] DreamPearl commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
DreamPearl commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r773936572
##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,121 @@
#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 DYNAMIC_ADDRESS = "test_dynamic_address";
+} // 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_receiver_open(proton::receiver& r) override {
+ std::map<proton::symbol,proton::value> m_sender({{proton::symbol("supported-dist-modes"), proton::value("move")}});
+ std::map<proton::symbol,proton::value> props = r.target().dynamic_properties();
+
+ ASSERT(r.target().dynamic());
+ ASSERT(r.target().address().empty());
+ ASSERT_EQUAL(m_sender, props);
+
+ proton::target_options opts;
+ std::map<proton::symbol, proton::value> m({{proton::symbol("supported-dist-modes"), proton::value("copy")}});
+ opts.address(DYNAMIC_ADDRESS);
+ opts.dynamic_properties(m);
+ if (r.uninitialized()) {
+ r.open(proton::receiver_options().target(opts));
+ }
+ listener.stop();
+ }
+};
+
+class test_send : public proton::messaging_handler {
+ private:
+ std::string url;
+
+ 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("supported-dist-modes"), proton::value("move")}});
Review comment:
I tried to make them 'constant' locally but I found the current version to be more readable. However, if you think adding constants will add more value, I can update it.
--
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
[GitHub] [qpid-proton] jiridanek commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r770837283
##########
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() {
+
+ std::string recv_address("127.0.0.1:0/test");
+ test_recv recv(recv_address);
+ proton::container c(recv);
+ std::thread thread_recv([&c]() -> void { c.run(); });
+
+ // wait until listener is ready
+ std::unique_lock<std::mutex> lk(m);
+ cv.wait(lk, [] { return listener_ready; });
+
+ std::string send_address =
+ "127.0.0.1:" + std::to_string(listener_port) + "/test";
+ test_send send(send_address);
+ proton::container(send).run();
+ thread_recv.join();
+
+ return 0;
Review comment:
there is a lot of tests that have this basic structure, but differ in details (depending on what function of API is currently being tested). I am wondering if they could be somehow refactored. But that is not here nor there for this PR.
--
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
[GitHub] [qpid-proton] DreamPearl commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
DreamPearl commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r773034946
##########
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:
For now, I have the server apply dynamic node properties other than the desired properties asked by the client. And verifying that they are picked up by the client.
pn_log: https://gist.github.com/DreamPearl/129d9d7d4f22b237269f6a5beefb227f
--
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
[GitHub] [qpid-proton] DreamPearl commented on pull request #346: [WIP] PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
DreamPearl commented on pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#issuecomment-994875591
@astitcher Updated the PR. In middle of writing tests.
--
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
[GitHub] [qpid-proton] DreamPearl commented on pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
DreamPearl commented on pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#issuecomment-999625841
@gemmellr I've made a few changes. Can you please take a look?
--
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
[GitHub] [qpid-proton] codecov-commenter edited a comment on pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#issuecomment-995011027
--
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
[GitHub] [qpid-proton] DreamPearl commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
DreamPearl commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r774064957
##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,121 @@
#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 DYNAMIC_ADDRESS = "test_dynamic_address";
+} // 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_receiver_open(proton::receiver& r) override {
+ std::map<proton::symbol,proton::value> m_sender({{proton::symbol("supported-dist-modes"), proton::symbol("move")}});
+ std::map<proton::symbol,proton::value> props = r.target().dynamic_properties();
+
+ ASSERT(r.target().dynamic());
+ ASSERT(r.target().address().empty());
+ ASSERT_EQUAL(m_sender, props);
+
+ proton::target_options opts;
+ std::map<proton::symbol, proton::value> m({{proton::symbol("supported-dist-modes"), proton::symbol("copy")}});
+ opts.address(DYNAMIC_ADDRESS);
+ opts.dynamic_properties(m);
+ if (r.uninitialized()) {
Review comment:
Ah, it was not needed as you mentioned. Removed it. Thanks!
--
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
[GitHub] [qpid-proton] DreamPearl commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
DreamPearl commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r773935495
##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,121 @@
#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 DYNAMIC_ADDRESS = "test_dynamic_address";
+} // 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_receiver_open(proton::receiver& r) override {
+ std::map<proton::symbol,proton::value> m_sender({{proton::symbol("supported-dist-modes"), proton::value("move")}});
Review comment:
Got it, done!
--
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
[GitHub] [qpid-proton] DreamPearl commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
DreamPearl commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r774051417
##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,121 @@
#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 DYNAMIC_ADDRESS = "test_dynamic_address";
+} // 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_receiver_open(proton::receiver& r) override {
+ std::map<proton::symbol,proton::value> m_sender({{proton::symbol("supported-dist-modes"), proton::symbol("move")}});
+ std::map<proton::symbol,proton::value> props = r.target().dynamic_properties();
+
+ ASSERT(r.target().dynamic());
+ ASSERT(r.target().address().empty());
Review comment:
```
void on_receiver_open(proton::receiver& r) override {
...
ASSERT(r.target().address().empty());
const char *address = pn_terminus_get_address(unwrap(r.target()));
ASSERT(NULL == address);
ASSERT_EQUAL(m_sender, props);
...
}
};
```
If we don't use the address method, then I guess we have to do something like the above. That means we will be using the C function in the C++ binding test, is that something we can/should do? As I don't see it being done in any other C++ test.
Also, I am getting an error in the above snippet.
```
...
[100%] Linking CXX executable link_test
/usr/bin/ld: CMakeFiles/link_test.dir/src/link_test.cpp.o: undefined reference to symbol 'pn_terminus_get_address'
...
```
Alternatively, we may test this behavior under C tests (Maybe this is what you meant above).
--
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
[GitHub] [qpid-proton] gemmellr commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r771394080
##########
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:
A dynamic consumer is by far the primary use case for dynamic, so if only having one test it would be that (essentially the opposite of this test). Having one test for a consumer, and another for a producer (from the client perspective) wouldnt be a bad idea, they are separate things, but dynamic senders are rare.
The test doesnt actually do all the steps I'd say it should. Its possible another test is doing some of them somewhere, or that its just not tested for the cpp binding. It should use a 'dynamic' source/target and verify the behaviour in full, right now it isnt, but is instead attaching to a known address. The 'dynamic' handling involves the original peer not setting an address, and the other peer generating an address for the dynamic node and supplying its detail back to the original side in its reply attach, which the requesting peer can then read to determine the name of the dynamic node created for it. The dynamic-node-properties adds to that mechanism by letting the requester also ask for certain node details to be applied. The generating peer can always use that same field to indicate properties actually in effect for the dynamic node it created.
The test only looks to verify that the node properties were sent [illegally due to not setting dynamic] from requestor to the other side. It doesnt check it can handle reading the details that come back, i.e to verify any dynamic address was given, or that any dynamic-node-properties arrived.
As an example, here is a Java test verifying the dynamic flag and address propagation aspects (it doesnt test the dynamic node properties) for a dynamic consumer:
https://github.com/vert-x3/vertx-proton/blob/4.2.2/src/test/java/io/vertx/proton/ProtonClientTest.java#L468-L545
I dont know if the 'built in' handshaking bits in proton-cpp that are opening the link in your test will inspect the dynamic flag and generate an address by themselves for the attach. If they do the test could just at least verify there is a generated value. If not, or better still, the test could set a generated address at the server side and check the client side can pick it up when its told the link opened. It could do the same for the dynamic node properties, e.g have the server apply something other than originally asked for, verify they are picked up too at the client once the link opens.
--
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
[GitHub] [qpid-proton] gemmellr commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r771380896
##########
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:
When being the 'requesting client' party asking for a consumer link to attach and have a dynamic node be created by setting the dynamic flag [and optionally setting dynamic node properties] that is done via the _source_, as it is requesting somewhere to consume from is dynamically created for it. The other side would read the source details requested by the requesting peer and action things, creating a node, giving it an address, and responding with its details. When it is sending its 'reply' attach it sets the live details that now exist also via the source (which from its perspective is now the local side, which it is authoritative on). The original peer can then read those details to establish the address and properties of the created node.
For a producer, it is similar but the _target_ is where the details will be set, as the original peer is requesting that somewhere to send to is dynamically created for it. The other peer will action this and respond with details via the _target_ to indicate what actually exists.
--
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
[GitHub] [qpid-proton] astitcher commented on a change in pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties
Posted by GitBox <gi...@apache.org>.
astitcher commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r773914859
##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,121 @@
#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 DYNAMIC_ADDRESS = "test_dynamic_address";
+} // 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_receiver_open(proton::receiver& r) override {
+ std::map<proton::symbol,proton::value> m_sender({{proton::symbol("supported-dist-modes"), proton::value("move")}});
Review comment:
Per the standard I think 'move' needs to be a symbol too. I think this will create an AMQP string value of 'move' which is not what the standard requests.
##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,121 @@
#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 DYNAMIC_ADDRESS = "test_dynamic_address";
+} // 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_receiver_open(proton::receiver& r) override {
+ std::map<proton::symbol,proton::value> m_sender({{proton::symbol("supported-dist-modes"), proton::value("move")}});
+ std::map<proton::symbol,proton::value> props = r.target().dynamic_properties();
+
+ ASSERT(r.target().dynamic());
+ ASSERT(r.target().address().empty());
+ ASSERT_EQUAL(m_sender, props);
+
+ proton::target_options opts;
+ std::map<proton::symbol, proton::value> m({{proton::symbol("supported-dist-modes"), proton::value("copy")}});
Review comment:
As above I think this should be ```proton::value(proton::symbol("copy"))```
##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,121 @@
#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 DYNAMIC_ADDRESS = "test_dynamic_address";
+} // 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_receiver_open(proton::receiver& r) override {
+ std::map<proton::symbol,proton::value> m_sender({{proton::symbol("supported-dist-modes"), proton::value("move")}});
+ std::map<proton::symbol,proton::value> props = r.target().dynamic_properties();
+
+ ASSERT(r.target().dynamic());
+ ASSERT(r.target().address().empty());
+ ASSERT_EQUAL(m_sender, props);
+
+ proton::target_options opts;
+ std::map<proton::symbol, proton::value> m({{proton::symbol("supported-dist-modes"), proton::value("copy")}});
+ opts.address(DYNAMIC_ADDRESS);
+ opts.dynamic_properties(m);
+ if (r.uninitialized()) {
+ r.open(proton::receiver_options().target(opts));
+ }
+ listener.stop();
+ }
+};
+
+class test_send : public proton::messaging_handler {
+ private:
+ std::string url;
+
+ 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("supported-dist-modes"), proton::value("move")}});
Review comment:
As above - maybe it would be better to have constants for move and copy symbols that can be used in this code to avoid repeating ```proton::value(proton::symbol("move"))``` etc. actually perhaps ```proton::symbol("move")``` may be enough as I think it should automatically convert to ```proton::value``
--
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