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

Handle duplicate node names by including zenoh session id in liveliness tokens #87

Merged
merged 6 commits into from
Jan 2, 2024

Conversation

Yadunund
Copy link
Member

@Yadunund Yadunund commented Dec 21, 2023

This PR addresses #84.

It so does so by including the zenoh session id in the liveliness token.
The token is updated to:
<ADMIN_SPACE>/<domainid>/<id>/<entity>/<namespace>/<nodename> where <id> is meant to be a unique ID to identify this entity. However, until the zenoh API exposes the globally unique IDs for each participant/entity, this id is currently set to the zenoh session's id with elements concatenated into a string using '.' as separator.

The NodeMap data structure is updated to an std::multimap to allow storing multiple nodes with the same name in the same namespace. Codebase is updated accoridngly.

Now after running two instances of /talker, running the command ros2 node list should correctly list two separate /talker nodes.
Note: Make sure to kill all ros2 nodes and daemons before trying this.

yadunund@ubuntu-22-04:~/ws_rmw$ ros2 node list
WARNING: Be aware that there are nodes in the graph that share an exact name, which can have unintended side effects.
/talker
/talker

Copy link
Collaborator

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've left two minor things to fix, this otherwise looks good to me.

rmw_zenoh_cpp/src/detail/graph_cache.cpp Outdated Show resolved Hide resolved
@@ -83,6 +83,18 @@ static const std::unordered_map<std::string, EntityType> str_to_entity = {
{CLI_STR, EntityType::Client}
};

std::string convert(z_id_t id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about:

Suggested change
std::string convert(z_id_t id)
std::string zid_to_str(z_id_t id)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done 6d3acd3

Yadunund and others added 2 commits January 2, 2024 23:54
@clalancette clalancette merged commit 0663919 into rolling Jan 2, 2024
5 checks passed
@delete-merged-branch delete-merged-branch bot deleted the yadu/zid_in_tokens branch January 2, 2024 16:35
Yadunund added a commit that referenced this pull request Jan 12, 2024
…ss tokens (#87)

* Include zenoh session id in liveliness tokens

* Switch to multimap for NodeMap

Signed-off-by: Yadunund <[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.

2 participants