Skip to content

Commit

Permalink
Remove Entity::node_info.
Browse files Browse the repository at this point in the history
Because it returned a reference, it is difficult to
tell the lifetime of it.  It would also be hard to add
in locking in the future.  Since we are reaching through
it in most cases, just return some data directly.

Signed-off-by: Chris Lalancette <[email protected]>
  • Loading branch information
clalancette committed Nov 27, 2023
1 parent 7c79ff1 commit ae0b4d9
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 25 deletions.
34 changes: 17 additions & 17 deletions rmw_zenoh_cpp/src/detail/graph_cache.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,9 @@ void GraphCache::parse_put(const std::string & keyexpr)
[&](const Entity & entity) -> std::shared_ptr<GraphNode>
{
auto graph_node = std::make_shared<GraphNode>();
graph_node->ns_ = entity.node_info().ns_;
graph_node->name_ = entity.node_info().name_;
graph_node->enclave_ = entity.node_info().enclave_;
graph_node->ns_ = entity.node_namespace();
graph_node->name_ = entity.node_name();
graph_node->enclave_ = entity.node_enclave();

if (!entity.topic_info().has_value()) {
// Token was for a node.
Expand All @@ -161,26 +161,26 @@ void GraphCache::parse_put(const std::string & keyexpr)
std::lock_guard<std::mutex> lock(graph_mutex_);

// If the namespace did not exist, create it and add the node to the graph and return.
auto ns_it = graph_.find(entity.node_info().ns_);
auto ns_it = graph_.find(entity.node_namespace());
if (ns_it == graph_.end()) {
std::unordered_map<std::string, GraphNodePtr> node_map = {
{entity.node_info().name_, make_graph_node(entity)}};
graph_.insert(std::make_pair(entity.node_info().ns_, std::move(node_map)));
{entity.node_name(), make_graph_node(entity)}};
graph_.insert(std::make_pair(entity.node_namespace(), std::move(node_map)));
RCUTILS_LOG_WARN_NAMED(
"rmw_zenoh_cpp", "Added node /%s to a new namespace %s in the graph.",
entity.node_info().name_.c_str(),
entity.node_info().ns_.c_str());
entity.node_name().c_str(),
entity.node_namespace().c_str());
return;
}

// Add the node to the namespace if it did not exist and return.
auto node_it = ns_it->second.find(entity.node_info().name_);
auto node_it = ns_it->second.find(entity.node_name());
if (node_it == ns_it->second.end()) {
ns_it->second.insert(std::make_pair(entity.node_info().name_, make_graph_node(entity)));
ns_it->second.insert(std::make_pair(entity.node_name(), make_graph_node(entity)));
RCUTILS_LOG_WARN_NAMED(
"rmw_zenoh_cpp", "Added node /%s to an existing namespace %s in the graph.",
entity.node_info().name_.c_str(),
entity.node_info().ns_.c_str());
entity.node_name().c_str(),
entity.node_namespace().c_str());
return;
}

Expand Down Expand Up @@ -304,13 +304,13 @@ void GraphCache::parse_del(const std::string & keyexpr)
std::lock_guard<std::mutex> lock(graph_mutex_);

// If namespace does not exist, ignore the request.
auto ns_it = graph_.find(entity.node_info().ns_);
auto ns_it = graph_.find(entity.node_namespace());
if (ns_it == graph_.end()) {
return;
}

// If the node does not exist, ignore the request.
auto node_it = ns_it->second.find(entity.node_info().name_);
auto node_it = ns_it->second.find(entity.node_name());
if (node_it == ns_it->second.end()) {
return;
}
Expand All @@ -326,16 +326,16 @@ void GraphCache::parse_del(const std::string & keyexpr)
"rmw_zenoh_cpp",
"Received liveliness token to remove node /%s from the graph before all pub/subs for this "
"node have been removed. Report this issue.",
entity.node_info().name_.c_str()
entity.node_name().c_str()
);
// TODO(Yadunund): Iterate through the nodes pubs_ and subs_ and decrement topic count in
// graph_topics_.
}
ns_it->second.erase(entity.node_info().name_);
ns_it->second.erase(entity.node_name());
RCUTILS_LOG_WARN_NAMED(
"rmw_zenoh_cpp",
"Removed node /%s from the graph.",
entity.node_info().name_.c_str()
entity.node_name().c_str()
);
return;
}
Expand Down
22 changes: 16 additions & 6 deletions rmw_zenoh_cpp/src/detail/liveliness_utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ Entity::Entity(
* <ADMIN_SPACE>/<domainid>/<entity>/<namespace>/<nodename>/<topic_name>/<topic_type>/<topic_qos>
*/
std::stringstream token_ss;
const auto & ns = node_info_.ns_;
token_ss << ADMIN_SPACE << "/" << node_info_.domain_id_ << "/" << entity_to_str.at(type_) << ns;
const std::string & ns = node_info_.ns_;
token_ss << ADMIN_SPACE << "/" << node_info_.domain_id_ << "/" << entity_to_str.at(type_) << node_info_.ns_;
// An empty namespace from rcl will contain "/" but zenoh does not allow keys with "//".
// Hence we add an "_" to denote an empty namespace such that splitting the key
// will always result in 5 parts.
Expand Down Expand Up @@ -230,7 +230,8 @@ std::optional<Entity> Entity::make(const std::string & keyexpr)

// Get the entity, ie NN, MP, MS, SS, SC.
std::string & entity_str = parts[2];
const auto entity_it = str_to_entity.find(entity_str);
std::unordered_map<std::string, EntityType>::const_iterator entity_it =
str_to_entity.find(entity_str);
if (entity_it == str_to_entity.end()) {
RCUTILS_LOG_ERROR_NAMED(
"rmw_zenoh_cpp",
Expand Down Expand Up @@ -270,10 +271,19 @@ const EntityType & Entity::type() const
return this->type_;
}

///=============================================================================
const NodeInfo & Entity::node_info() const
std::string Entity::node_namespace() const
{
return this->node_info_.ns_;
}

std::string Entity::node_name() const
{
return this->node_info_.name_;
}

std::string Entity::node_enclave() const
{
return this->node_info_;
return this->node_info_.enclave_;
}

///=============================================================================
Expand Down
7 changes: 5 additions & 2 deletions rmw_zenoh_cpp/src/detail/liveliness_utils.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,11 @@ class Entity
/// Get the entity type.
const EntityType & type() const;

/// Get the node_info.
const NodeInfo & node_info() const;
std::string node_namespace() const;

std::string node_name() const;

std::string node_enclave() const;

/// Get the topic_info.
const std::optional<TopicInfo> & topic_info() const;
Expand Down

0 comments on commit ae0b4d9

Please sign in to comment.