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

Add rmw count clients, services #334

Merged
merged 3 commits into from
Feb 9, 2023
Merged

Conversation

leeminju531
Copy link
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
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
Contributor

Wait out for CI

@methylDragon
Copy link
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
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
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
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
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
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.

rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
Signed-off-by: leeminju531 <dlalswn531@naver.com>
@methylDragon
Copy link
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.

None yet

4 participants