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

Subscribe to topics using the common offered QoS #343

Merged
merged 8 commits into from
Apr 2, 2020

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Apr 1, 2020

Part of #125
Moves https://github.com/ros-tooling/aws-roadmap/issues/216 to Done

Subscribe to topics using the common offered QoS if there is one, with sufficient warning in the cases where we can't.

As we discussed in the original design, we will not try to resolve compatibility at all. So, if all publishers at the time of subscription offer the same QoS profile (likely vast majority of cases), then that profile will be used to subscribe.

However, if there are different profiles being offered for a topic, fallback to the preexisting default. This means rosbag2 acts the same for these cases as it does before this PR. But, now we print a warning about it.

If new publishers join after our subscription was created, and they offer a different QoS profile than we used to subscribe, also print a warning about that case because it may result in loss of data.

Signed-off-by: Emerson Knapp [email protected]

…th sufficient warning in the cases where we can't

Signed-off-by: Emerson Knapp <[email protected]>
@emersonknapp emersonknapp changed the title Subscribe to topics using the common offered QoS [WIP] Subscribe to topics using the common offered QoS Apr 1, 2020
@emersonknapp emersonknapp changed the title [WIP] Subscribe to topics using the common offered QoS Subscribe to topics using the common offered QoS Apr 2, 2020
@emersonknapp emersonknapp marked this pull request as ready for review April 2, 2020 01:37
Copy link
Contributor

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

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

LGTM, few very minor nits.

rosbag2_transport/src/rosbag2_transport/recorder.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/recorder.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/recorder.hpp Outdated Show resolved Hide resolved
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
@emersonknapp
Copy link
Collaborator Author

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

@emersonknapp
Copy link
Collaborator Author

The windows build is again the known test failure

@emersonknapp emersonknapp merged commit 8ba8506 into master Apr 2, 2020
@delete-merged-branch delete-merged-branch bot deleted the emersonknapp/subscribe-with-qos branch April 2, 2020 22:27
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