Skip to content

Disable adaptive qos subscription for now #348

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
Merged
emersonknapp merged 2 commits into from
Apr 3, 2020
Merged

Disable adaptive qos subscription for now #348

emersonknapp merged 2 commits into from
Apr 3, 2020

Conversation

@emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Apr 3, 2020

until we get it working on cyclone

Unblocks #345

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

@emersonknapp emersonknapp mentioned this pull request Apr 3, 2020
Merged
Karsten1987
Karsten1987 approved these changes Apr 3, 2020
View reviewed changes
Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

lgtm.

I recommend running two individual CI jobs - one with Fast-RTPS and one with Cyclone - demonstrating that both succeed.

Regarding the DDS vendor matrix, I would say it should suffice to enable it on rosbag2_transport as well as rosbag2_tests. The rest should technically be communication independent. We shouldn't have to stretch out the test run unnecessary long.

Do you want to pick that up in a separate PR or in this one?

@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Apr 3, 2020

I think I'll do that in a separate PR. I'm working on it now using the call_for_each_rmw_implementation CMake macro (right approach?) but I'd like to get your changes unblocked as quickly and cleanly as possible.

I will just enable it on rosbag2_transport and rosbag2_tests, I agree that the rest should be independent of transport

@Karsten1987
Copy link
Collaborator

Karsten1987 commented Apr 3, 2020

I'm working on it now using the call_for_each_rmw_implementation CMake macro (right approach?)

yes!

@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Apr 3, 2020

FastRTPS run

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

CycloneDDS run

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

@emersonknapp
Fix inverted disabling logic
48411ab 
Signed-off-by: Emerson Knapp <[email protected]>
@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Apr 3, 2020

FastRTPS

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

CycloneDDS

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

@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Apr 3, 2020

The one failing test is the remaining unrelated zstd compression splitting. Merging now.

@emersonknapp emersonknapp merged commit 53de149 into master Apr 3, 2020
@delete-merged-branch delete-merged-branch bot deleted the emersonknapp/disable-adaptive-qos-for-now branch April 3, 2020 04:32
@Karsten1987 Karsten1987 mentioned this pull request Apr 3, 2020
Closed
21 tasks
@piraka9011 piraka9011 mentioned this pull request Apr 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Karsten1987 Karsten1987 Karsten1987 approved these changes

@thomas-moulard thomas-moulard Awaiting requested review from thomas-moulard

@zmichaels11 zmichaels11 Awaiting requested review from zmichaels11

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@emersonknapp @Karsten1987