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.

Sign up for GitHub

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

Jump to bottom

[foxy backport] Make service wait for response reader (#390) #412

Merged
merged 1 commit into from
Jul 21, 2020
Merged

[foxy backport] Make service wait for response reader (#390) #412

merged 1 commit into from
Jul 21, 2020

Conversation

jacobperron
Copy link
Member

Backport #390 to Foxy.

Note, this breaks ABI compatibility, but only below the RMW layer. IMO, the breakage is acceptable considering the improvement to service discovery.

I'm still checking if this change is backports compatible over the wire.

@MiguelCompany @jacobperron
Make service wait for response reader (#390)
d1942c0 
* Sending response subscriber guid with request.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Server ensures that response reader is matched.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Addressing review.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Linters

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Using unordered_set

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Linters

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Additional checks on rmw_service_server_is_available.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Suggestions on guid_utils

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Added TODO mentioning DDS-RPC.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* linters again.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@jacobperron jacobperron requested review from ivanpauno and hidmic and removed request for ivanpauno July 21, 2020 18:26
ivanpauno
ivanpauno approved these changes Jul 21, 2020
View reviewed changes
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

IMO, the breakage is acceptable considering the improvement to service discovery.

I agree.
We should comment the breakage in the release notes, clarifying the it only affects users making use of FastRTPS native handles.

I'm still checking if this change is backports compatible over the wire.

I think we should check the following:

  • A client of the new version can make requests to a service of the old version.
  • A client of the old version can make requests to a service of the new version.
  • Service and clients of the old version are listed correctly by a ros2cli tool of the new version.
  • Service and clients of the new version are listed correctly by a ros2cli tool of the old version.

That should be enough to confirm wire compatibility.

@jacobperron
Copy link
Member Author

  • A client of the new version can make requests to a service of the old version.
  • A client of the old version can make requests to a service of the new version.
  • Service and clients of the old version are listed correctly by a ros2cli tool of the new version.
  • Service and clients of the new version are listed correctly by a ros2cli tool of the old version.

All of these checks LGTM; I did checks on my local machine and between two hosts over a VPN.

@jacobperron jacobperron merged commit 75554ec into foxy Jul 21, 2020
@delete-merged-branch delete-merged-branch bot deleted the jacob/backport_390 branch July 21, 2020 19:26
@jacobperron
Copy link
Member Author

Note, I've also backported #409 which fixes a compiler warning introduced by this change (#413)

@jacobperron jacobperron changed the title Make service wait for response reader (#390) [foxy backport] Make service wait for response reader (#390) Jul 21, 2020
@MiguelCompany
Copy link
Collaborator

We should comment the breakage in the release notes, clarifying the it only affects users making use of FastRTPS native handles.

This is only affecting rmw, so users making use of native handles will NOT be affected, right?

@jacobperron
Copy link
Member Author

This is only affecting rmw, so users making use of native handles will NOT be affected, right?

I think what @ivanpauno meant, is it only affects users making use of the handles in rmw_fastrtps (instead of the generic rmw structs). IIUC, only people using the structs CustomClientInfo and/or CustomServiceInfo will be affected. Does that sound right?

@MiguelCompany
Copy link
Collaborator

Does that sound right?

Perfect!

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/new-packages-for-foxy-fitzroy-2020-07-23/15570/2

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/new-packages-and-patch-release-for-ros-2-foxy-fitzroy-2020-08-07/15818/1

@JLBuenoLopez JLBuenoLopez mentioned this pull request Oct 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivanpauno ivanpauno ivanpauno approved these changes

@hidmic hidmic Awaiting requested review from hidmic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jacobperron @MiguelCompany @ros-discourse @ivanpauno