Skip to content

Add rmw count clients, services#334

Merged
methylDragon merged 3 commits intoros2:rollingfrom
leeminju531:rolling
Feb 9, 2023
Merged

Add rmw count clients, services#334
methylDragon merged 3 commits intoros2:rollingfrom
leeminju531:rolling

Conversation

@leeminju531
Copy link
Copy Markdown
Contributor

this PR is for service counts using it's name like topic.
Related to ros2/ros2cli#771

Signed-off-by: leeminju531 dlalswn531@naver.com

Signed-off-by: leeminju531 <dlalswn531@naver.com>
@leeminju531 leeminju531 marked this pull request as ready for review October 8, 2022 14:25
Signed-off-by: leeminju531 <dlalswn531@naver.com>
@leeminju531 leeminju531 marked this pull request as draft October 31, 2022 17:09
@leeminju531 leeminju531 marked this pull request as ready for review November 22, 2022 16:19
@leeminju531
Copy link
Copy Markdown
Contributor Author

F.Y.I

There is yet no reviewers in rmw_connextdds (ros2/rmw_connextdds#93)
I would be grateful if someone could confirm.

@methylDragon
Copy link
Copy Markdown
Contributor

Wait out for CI

@methylDragon
Copy link
Copy Markdown
Contributor

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Comment on lines +2993 to +2994
* This function returns the numbers of servers of a given service in the ROS graph,
* as discovered so far by the given node.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we actually support multiple servers on single service name? i believe that we can create the services on the same name, but how it works is really dependent on implementation. If this is supported, i would expect that it can also support load balancing.

IMO, multiple service servers on single topic is not officially supported. in that case, I am not sure what this is for.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@leeminju531 @methylDragon what do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The ROS2 design says

DDS currently does not have a ratified or implemented standard for request-response style RPC which could be used to implement the concept of services in ROS. There is currently an RPC specification being considered for ratification in the OMG DDS working group, and several of the DDS vendors have a draft implementation of the RPC API.

I think It's seem like none of the specification in this situation. I don't know if some supports like load balancing will be added in the future, As long as most vendors allow multiple servers on the same name, it seems like a necessary function even now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we actually support multiple servers on single service name? i believe that we can create the services on the same name, but how it works is really dependent on implementation. If this is supported, i would expect that it can also support load balancing.

It's not supported, but you can still create more than one.
I think that the default behavior is that all servers reply, no load balancing.
I'm not completely sure though, and I'm not sure if something breaks in that case.

Being able to get the number of service servers still seems okay.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It's not supported, but you can still create more than one.
I think that the default behavior is that all servers reply, no load balancing.

it seems that it depends on implementation, i just checked with Fast-DDS, it sometimes both servers reply, sometimes only one server replies. for doing this, i guess we need to consider more details, like how we can match the service type, how we can choose the server, how to load balance and so on. this could be new feature.

Being able to get the number of service servers still seems okay.

yeah, at least we can create multiple servers on the same service path.

Signed-off-by: leeminju531 <dlalswn531@naver.com>
@methylDragon
Copy link
Copy Markdown
Contributor

Posting this in here for reference too: ros2/rmw_implementation#208

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.

4 participants