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

Use client's reader guid for service introspection event gid. #781

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

fujitatomoya
Copy link
Collaborator

part of ros2/rmw#357

Copy link
Collaborator Author

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

@MiguelCompany what do you think about this change?

I am almost sure that using client's reader guid instead of writer guid here in the client for the service introspection event, will not affect any other behavior in rmw_fastrtps.

@fujitatomoya
Copy link
Collaborator Author

CI:

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

@MiguelCompany
Copy link
Collaborator

This is ok... if only using rmw_fastrtps_cpp.

Should we consider the case of the server and the client using different vendors?
What would the expectation be in that case?

I am ok with merging this, and addressing the cross-vendor case in a follow-up, but I would like @clalancette opinion first.

@fujitatomoya
Copy link
Collaborator Author

atm, i think that is okay not to consider cross-vendor communication and current code does not seem to care about that either. (https://docs.ros.org/en/rolling/Concepts/Intermediate/About-Different-Middleware-Vendors.html#cross-vendor-communication) i understand we necessarily do not want to break that compatibility with this., but rosbag2 service problem

without this uniqueness capability during a single request-reponse pair, it cannot tell which request is mapped to which response, so that rosbag2 cannot know which message needs to play.

is much bigger problem happening right now...

@fujitatomoya
Copy link
Collaborator Author

@clalancette @wjwwood what do you think?

@MiguelCompany
Copy link
Collaborator

i think that is okay not to consider cross-vendor communication

In fact, we would first need to have service interoperability between all the RMWs, which is currently not the case.

Approved, and CI green, so I feel free to merge

@MiguelCompany MiguelCompany merged commit 8fd6f3c into rolling Oct 3, 2024
3 checks passed
@MiguelCompany MiguelCompany deleted the fujitatomoya/service-event-gid-uniqueness branch October 3, 2024 08:17
@clalancette
Copy link
Contributor

Did we run CI on RHEL Debug for this? That was the case that caused all of the problems before.

@clalancette
Copy link
Contributor

clalancette commented Oct 3, 2024

It looks like not, here is a run. If this fails, we'll have to revert:

  • Linux-rhel Build Status

@clalancette
Copy link
Contributor

All right, all looks happy with this. We'll see what happens overnight.

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