Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Allow more flexible usage of XML QoS profiles#460

Merged
ivanpauno merged 25 commits intomasterfrom
ivanpauno/more-flexibility-in-qos-profiles
Oct 13, 2020
Merged

Allow more flexible usage of XML QoS profiles#460
ivanpauno merged 25 commits intomasterfrom
ivanpauno/more-flexibility-in-qos-profiles

Conversation

@ivanpauno
Copy link
Copy Markdown
Member

@ivanpauno ivanpauno commented Sep 21, 2020

It allows more flexibility through environment variables, checkout README.md for more details.

TODO: Write tests.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the in review Waiting for review (Kanban column) label Sep 21, 2020
@ivanpauno ivanpauno self-assigned this Sep 21, 2020
@ivanpauno
Copy link
Copy Markdown
Member Author

@jacobperron I was originally thinking to add this in a fork, but if the defaults aren't modified and flexibility is provided through environment variables, I guess we can merge the change on master.

@ivanpauno
Copy link
Copy Markdown
Member Author

BTW, this is ready for being reviewed. I opened it as a draft only because tests are missing.

ivanpauno and others added 7 commits September 30, 2020 12:01
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
@ivanpauno ivanpauno marked this pull request as ready for review September 30, 2020 16:18
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno requested a review from jacobperron October 8, 2020 14:53
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Copy link
Copy Markdown
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

I didn't exercise the feature, but it LGTM. One wording suggestion.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
@ivanpauno
Copy link
Copy Markdown
Member Author

@jacobperron I'm planning to write some tests before merging, to improve coverage.
We could kick a packaging build with this branch, I have done some manual testing and it's working fine.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Copy Markdown
Member Author

ivanpauno commented Oct 9, 2020

Tests added, CI (testing rcl, test_rmw_implementation, rmw_connext_cpp, rmw_connext_shared_cpp):

  • Linux Build Status
  • Linux-aarch64 Build Status (failed because rti Connext isn't available in aarch64 in our CI)
  • macOS Build Status
  • Windows Build Status

@ivanpauno ivanpauno requested a review from jacobperron October 9, 2020 22:05
@ivanpauno ivanpauno closed this Oct 9, 2020
@ivanpauno ivanpauno reopened this Oct 9, 2020
Copy link
Copy Markdown
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM, just some tiny nitpicks

ivanpauno and others added 2 commits October 13, 2020 11:18
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
@ivanpauno
Copy link
Copy Markdown
Member Author

Going in, thanks for the reviews @jacobperron !

@ivanpauno ivanpauno merged commit 2572187 into master Oct 13, 2020
@ivanpauno ivanpauno deleted the ivanpauno/more-flexibility-in-qos-profiles branch October 13, 2020 14:19
@mjcarroll
Copy link
Copy Markdown
Member

@ivanpauno There is a chance that this introduced flakiness in the nightly Windows builds. Do you mind taking a look and seeing if it was this PR? These are the tests in question: https://ci.ros2.org/job/nightly_win_rel/1721/testReport/

@ivanpauno
Copy link
Copy Markdown
Member Author

@ivanpauno There is a chance that this introduced flakiness in the nightly Windows builds. Do you mind taking a look and seeing if it was this PR? These are the tests in question: https://ci.ros2.org/job/nightly_win_rel/1721/testReport/

I don't see how this PR can make that test fail, it's completely unrelated.
That test is failing to find that some nodes were set up within a timeout, and this PR cannot affect node discovery (I would be extremely surprised if it somehow does).

Most likely, the test was already flaky.

@mjcarroll
Copy link
Copy Markdown
Member

I didn't think so, but I also hadn't seen these tests pop up in recent history.

I'll see if I can reproduce, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

in review Waiting for review (Kanban column)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants