Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finish GraphCache implementation for Pub/Sub #66

Merged
merged 12 commits into from
Nov 19, 2023
Merged

Conversation

Yadunund
Copy link
Member

@Yadunund Yadunund commented Nov 15, 2023

This PR:

  • Refactors how we store and update the GraphCache within each session. Specifically we move away from a YAML::Node to an unordered_map (although implementing a conversion from our custom map to YAML may be helpful for logging or transmitting the graph as a serialized YAML over the network).
  • Makes the switch to rely on liveliness tokens to broadcast changes to the ROS graph. The previous implementation of relying on put/del/get convey the changes is retained but commented out as it could be reused to support discovery with zenoh-pico.
  • Implements some basic topic/node introspection functions in rmw (the rest will be done in a separate PR).

Note: Without the router, nodes will not be able to discover each other since multicast discovery is disabled by default in the node's session config. Instead, nodes will receive discovery information about other peers via the router's gossip function.

@Yadunund Yadunund marked this pull request as ready for review November 16, 2023 11:35
Copy link
Collaborator

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great Job @Yadunund 🚀
I left a bunch of not-game-changing comments, ptal

rmw_zenoh_cpp/src/detail/graph_cache.cpp Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/graph_cache.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/graph_cache.cpp Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/graph_cache.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/graph_cache.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/graph_cache.cpp Show resolved Hide resolved
Comment on lines +421 to +437
auto erase_it = found_node->pubs.begin();
for (; erase_it != found_node->pubs.end(); ++erase_it) {
const auto & pub = *erase_it;
if (pub.topic == node->pubs.at(0).topic &&
pub.type == node->pubs.at(0).type &&
pub.qos == node->pubs.at(0).qos)
{
break;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use find_if here and there

Suggested change
auto erase_it = found_node->pubs.begin();
for (; erase_it != found_node->pubs.end(); ++erase_it) {
const auto & pub = *erase_it;
if (pub.topic == node->pubs.at(0).topic &&
pub.type == node->pubs.at(0).type &&
pub.qos == node->pubs.at(0).qos)
{
break;
}
}
auto erase_it = std::find_if(found_node->pubs.begin(), found_node->pubs.end(), [&node](const auto &pub) {
return pub.topic == node->pubs.at(0).topic &&
pub.type == node->pubs.at(0).type &&
pub.qos == node->pubs.at(0).qos;
});

Then proceed as you do:

        if (erase_it != found_node->pubs.end()) {
            // Found a matching element, erase it
            found_node->pubs.erase(erase_it);
            //...
            //...
            //...
        }

rmw_zenoh_cpp/src/detail/graph_cache.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/rmw_get_topic_names_and_types.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/graph_cache.cpp Show resolved Hide resolved
Yadunund and others added 11 commits November 17, 2023 14:54
* Switch to liveliness tokens

Signed-off-by: Yadunund <[email protected]>

* Use zc APIs instead of macros to resolve liveliness api issues

Signed-off-by: Yadunund <[email protected]>

---------

Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
When playing around with this I found that the types
weren't being properly filled in, and also weren't being
demangled.  Fix both of those things here, as well
as do a lot of cleanup.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Collaborator

I ended up rebasing this on the latest, as well as fixing some bugs that I found. I also addressed most of @francocipollone 's comments, with the exception of a switch to find_if. I'll take a closer look at that later.

However, with all of that done, ros2 node list and ros2 topic list -t both work entirely correctly for me. There is still an open TODO in there where a topic with multiple types on it won't be shown properly. To fix that, we'll need to revamp the way some of the data is put together. That's a project for next week.

@francocipollone
Copy link
Collaborator

I ended up rebasing this on the latest, as well as fixing some bugs that I found. I also addressed most of @francocipollone 's comments, with the exception of a switch to find_if. I'll take a closer look at that later.

However, with all of that done, ros2 node list and ros2 topic list -t both work entirely correctly for me. There is still an open TODO in there where a topic with multiple types on it won't be shown properly. To fix that, we'll need to revamp the way some of the data is put together. That's a project for next week.

Confirmed, it is working correctly. It seems it was my bad: I had some issues with the daemon. Basically, it was already running (I was working on another branch) and then when I checkouted this branch and built I hadn't stopped the daemon.

@Yadunund
Copy link
Member Author

Thanks for cleaning this up Chris! I'll work on massaging the cache data structures to support multiple types on the same topic in a separate PR to keep the diffs minimal. If both of you are okay with the code here, should we merge this in first?

Signed-off-by: Yadunund <[email protected]>
@clalancette
Copy link
Collaborator

Thanks for cleaning this up Chris! I'll work on massaging the cache data structures to support multiple types on the same topic in a separate PR to keep the diffs minimal. If both of you are okay with the code here, should we merge this in first?

Yeah, I think we should do that. The open issues here are 1) to switch to find_if as suggested by Franco, and 2) fix multiple types per topic. As long as we fix both of those in a follow-up, I'm good with merging this.

@Yadunund Yadunund merged commit fe77eef into rolling Nov 19, 2023
5 checks passed
@delete-merged-branch delete-merged-branch bot deleted the yadu/graph-cache branch November 19, 2023 15:55
Yadunund added a commit that referenced this pull request Nov 19, 2023
Yadunund added a commit that referenced this pull request Jan 12, 2024
* Add storage plugin config to router config

Signed-off-by: Yadunund <[email protected]>

* Fix non empty node namespace

Signed-off-by: Yadunund <[email protected]>

* Rely on unordered_map instead of yaml for graph cache

Signed-off-by: Yadunund <[email protected]>

* Update graph cache with publisher data

Signed-off-by: Yadunund <[email protected]>

* Implement basic publisher introspection

Signed-off-by: Yadunund <[email protected]>

* Switch to liveliness tokens (#67)

* Switch to liveliness tokens

Signed-off-by: Yadunund <[email protected]>

* Use zc APIs instead of macros to resolve liveliness api issues

Signed-off-by: Yadunund <[email protected]>

---------

Signed-off-by: Yadunund <[email protected]>

* Cleanup yaml-cpp

Signed-off-by: Yadunund <[email protected]>

* Cleanup previous graph cache impl

Signed-off-by: Yadunund <[email protected]>

* Refactor topic cache

Signed-off-by: Yadunund <[email protected]>

* Implement liveliness tokens for subscriptions and update graph

Signed-off-by: Yadunund <[email protected]>

* Fix types and type mangling.

When playing around with this I found that the types
weren't being properly filled in, and also weren't being
demangled.  Fix both of those things here, as well
as do a lot of cleanup.

Signed-off-by: Chris Lalancette <[email protected]>

* Update README

Signed-off-by: Yadunund <[email protected]>

---------

Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants