Add 'best available' QoS policies #320

jacobperron · 2022-04-15T23:48:48Z

Resolves #304
Alternative to #319

The best available policy should select the highest level of service for the QoS setting while matching with the majority of endpoints.
For example, in the case of a DDS middleware subscription, this means:

Prefer reliable reliability if all existing publishers on the same topic are reliable, otherwise use best effort.
Prefer transient local durability if all existing publishers on the same topic are transient local, otherwise use volatile.
Prefer manual by topic liveliness if all existing publishers on the same topic are manual by topic, otherwise use automatic.
Use a deadline that is equal to the largest deadline of existing publishers on the same topic.
Use a liveliness lease duration that is equal to the largest lease duration of existing publishers on the same topic.

Connected PRs:

CI:

Linux
Linux-aarch64
Windows

I've created branches named jacob/qos_best_available from PRs which are not using that name so CI will pick up the relevant changes.

The best available policy should select the highest level of service for the QoS setting while matching with the majority of endpoints. For example, in the case of a DDS middleware subscription, this means: - Prefer reliable reliability if all existing publishers on the same topic are reliable, otherwise use best effort. - Prefer transient local durability if all existing publishers on the same topic are transient local, otherwise use volatile. - Prefer manual by topic liveliness if all existing publishers on the same topic are manual by topic, otherwise use automatic. - Use a deadline that is equal to the largest deadline of existing publishers on the same topic. - Use a liveliness lease duration that is equal to the largest lease duration of existing publishers on the same topic. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2022-04-15T23:56:09Z

The implementation for DDS middlewares is in ros2/rmw_dds_common#60
I've integrated the feature into rmw_fastrtps in ros2/rmw_fastrtps#598

I still need to:

integrate feature into rmw_cyclonedds and rmw_connextdds
add some integration tests (e.g. in system_tests).
find and update places above rcl that might care about the new enum values.

wjwwood

lgtm

One comment about the naming, "best" seems natural, but technically there's no reason to consider some of these better than the alternative.

For example, we could say RMW_QOS_POLICY_RELIABILITY_RELIABLE_IF_POSSIBLE instead of RMW_QOS_POLICY_RELIABILITY_BEST_AVAILABLE and it would be more accurate I think. I'm not sold on that name really, but "best" is definitely ambiguous to me, especially for things like duration and lifespace, etc. Not a blocker, just wanted to bring it up.

rmw/include/rmw/types.h

ivanpauno

The proposed interface sounds pretty reasonable.

We should really document that this options should be used with care.
Abusing it can create issues of qos profiles changing randomly when launching a bunch of nodes together (because of discovery timing).

rmw/include/rmw/types.h

jacobperron · 2022-04-19T19:30:12Z

"best" seems natural, but technically there's no reason to consider some of these better than the alternative.

True. It could be misleading to users who may think this is the "best" option. While a name like RMW_QOS_POLICY_RELIABILITY_RELIABLE_IF_POSSIBLE makes sense for reliability, durability, and liveliness, what would we call the new deadline and lease duration sentinel values? Maybe something like RMW_QOS_DEADLINE_MATCH_ENDPOINTS?

jacobperron · 2022-04-19T20:45:04Z

I think there is something nice about using the same term for all policy types, e.g. how should we name the functions introduced in ros2/rmw_dds_common#60?

jacobperron · 2022-04-19T20:49:03Z

Maybe instead of "best available" we call it "highest available" (as in highest level of service, while matching the most endpoints)?

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2022-04-19T21:21:09Z

We should really document that this options should be used with care.

PTAL at the latest changes to the documentation.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

If users set certain a QoS policy to 'best available', then the middleware will query the graph for endpoint info before creating a subscription or publisher. A QoS policy will be selected such that is matches the majority of endpoints while maintaining the highest level of service possible. Connects to ros2/rmw#320 Signed-off-by: Jacob Perron <jacob@openrobotics.org>

If users set a policy as 'best available', then the middleware will pick a policy that is most compatible with the current set of discovered endpoints while maintaining the highest level of service possible. For details about the expected behavior, see connected changes: - ros2/rmw#320 - ros2/rmw_dds_common#60 Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Testing feature added in ros2/rmw#320 for all supported rmw implementations. Test creating a subscription and publisher with 'best available' policies and confirm the actual QoS policies are what we expect (adapting to an existing endpoint). Signed-off-by: Jacob Perron <jacob@openrobotics.org>

ivanpauno

The only open discussion is #320 (comment), otherwise it looks good to me

wjwwood

A few comments, but generally lgtm, nothing blocking.

rmw/include/rmw/types.h

* Avoid changing existing enum values * Clarify that actual reported QoS may be best effort or other values Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

In case users are relying on integer values. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

wjwwood

lgtm

After thinking about it, I can't come up with a better name than "best available", we'll just want to document it well.

If the other discussions are resolved, then this is g2g for me.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2022-04-26T00:21:37Z

I added a note about services: 492c48f

jacobperron · 2022-04-26T23:32:59Z

CI is passing for all connected PRs 🎉 #320 (comment)
I think the only thing left to resolve is the comment on this ticket: ros2/rmw_connextdds#84 (comment)

j-rivero · 2022-04-29T17:12:36Z

Run an smoke CI on ci_linux with --packages-up-to test_communication test_quality_of_service to check how things are in all the open PRs after latest changes on ros2/rmw_connextdds#85. I've used this ros2.repos file.
CI is passing there 🎉 @jacobperron a final look to that rmw_connextdds PR and we should be in a good moment to run the full CI again I think.

If users set a policy as 'best available', then the middleware will pick a policy that is most compatible with the current set of discovered endpoints while maintaining the highest level of service possible. For details about the expected behavior, see connected changes: - ros2/rmw#320 - ros2/rmw_dds_common#60 Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Expose 'best available' QoS policies If users set certain a QoS policy to 'best available', then the middleware will query the graph for endpoint info before creating a subscription or publisher. A QoS policy will be selected such that is matches the majority of endpoints while maintaining the highest level of service possible. Connects to ros2/rmw#320 * Add BEST_AVAILABLE to QoSPresetProfiles enum Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Add tests for 'best available' QoS policies Testing feature added in ros2/rmw#320 for all supported rmw implementations. Test creating a subscription and publisher with 'best available' policies and confirm the actual QoS policies are what we expect (adapting to an existing endpoint). * Add tests for services and clients Signed-off-by: Jacob Perron <jacob@openrobotics.org>

The best available policy should select the highest level of service for the QoS setting while matching with the majority of endpoints. For example, in the case of a DDS middleware subscription, this means: - Prefer reliable reliability if all existing publishers on the same topic are reliable, otherwise use best effort. - Prefer transient local durability if all existing publishers on the same topic are transient local, otherwise use volatile. - Prefer manual by topic liveliness if all existing publishers on the same topic are manual by topic, otherwise use automatic. - Use a deadline that is equal to the largest deadline of existing publishers on the same topic. - Use a liveliness lease duration that is equal to the largest lease duration of existing publishers on the same topic. * Add note about durability behavior with transient local publishers * Add note about getting actual QoS policy * Make best available enum value last to minimize disruption In case users are relying on integer values. * Document behavior for services Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron requested review from wjwwood and ivanpauno April 15, 2022 23:48

This was referenced Apr 15, 2022

Handle 'best_available' QoS policies ros2/rmw_fastrtps#598

Merged

Add functions for resolving 'best available' QoS policies ros2/rmw_dds_common#60

Merged

Add functions to get a compatible QoS profiles #319

Closed

wjwwood reviewed Apr 16, 2022

View reviewed changes

rmw/include/rmw/types.h Outdated Show resolved Hide resolved

rmw/include/rmw/types.h Outdated Show resolved Hide resolved

rmw/include/rmw/types.h Outdated Show resolved Hide resolved

rmw/include/rmw/types.h Outdated Show resolved Hide resolved

ivanpauno approved these changes Apr 18, 2022

View reviewed changes

rmw/include/rmw/types.h Outdated Show resolved Hide resolved

jacobperron added 3 commits April 19, 2022 14:08

Improve documentation

f6d2357

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Add note about durability behavior with transient local publishers

9988e82

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Add note about getting actual QoS policy

cf5bcab

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Improve documentation of 'best available' profile

f10a8a3

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

This was referenced Apr 19, 2022

Expose 'best available' QoS policies ros2/rclpy#928

Merged

Automatic/adaptive subscription QoS ros2/rclcpp#1868

Open

jacobperron mentioned this pull request Apr 19, 2022

Add 'best available' QoS enum values and methods ros2/rclcpp#1920

Merged

jacobperron mentioned this pull request Apr 20, 2022

Add tests for 'best available' QoS policies ros2/system_tests#501

Merged

jacobperron requested a review from j-rivero April 20, 2022 01:37

This was referenced Apr 20, 2022

Add functions to get a compatible QoS profiles ros2/rcl#976

Closed

Add API for getting compatible QoS profiles ros2/rmw_implementation#205

Closed

Add functions to get compatible QoS profiles ros2/rmw_dds_common#59

Closed

ivanpauno approved these changes Apr 20, 2022

View reviewed changes

wjwwood approved these changes Apr 21, 2022

View reviewed changes

rmw/include/rmw/types.h Outdated Show resolved Hide resolved

rmw/include/rmw/types.h Outdated Show resolved Hide resolved

rmw/include/rmw/types.h Outdated Show resolved Hide resolved

This was referenced Apr 22, 2022

Handle 'best_available' QoS policies ros2/rmw_connextdds#84

Closed

Handle 'best_available' QoS policies ros2/rmw_cyclonedds#389

Merged

jacobperron added 3 commits April 22, 2022 16:03

Change liveliness enum value and update documentation

069042b

* Avoid changing existing enum values * Clarify that actual reported QoS may be best effort or other values Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Fix comma

6dc811d

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Make best available enum value last to minimize disruption

95241ad

In case users are relying on integer values. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

wjwwood approved these changes Apr 25, 2022

View reviewed changes

Document behavior for services

492c48f

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron mentioned this pull request May 2, 2022

Handle 'best_available' QoS policies in common ros2/rmw_connextdds#85

Merged

j-rivero approved these changes May 3, 2022

View reviewed changes

jacobperron merged commit ac633c0 into master May 3, 2022

delete-merged-branch bot deleted the jacob/qos_best_available branch May 3, 2022 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'best available' QoS policies #320

Add 'best available' QoS policies #320

jacobperron commented Apr 15, 2022 •

edited

Loading

jacobperron commented Apr 15, 2022 •

edited

Loading

wjwwood left a comment

ivanpauno left a comment

jacobperron commented Apr 19, 2022

jacobperron commented Apr 19, 2022

jacobperron commented Apr 19, 2022

jacobperron commented Apr 19, 2022

ivanpauno left a comment

wjwwood left a comment

wjwwood left a comment

jacobperron commented Apr 26, 2022

jacobperron commented Apr 26, 2022

j-rivero commented Apr 29, 2022

Add 'best available' QoS policies #320

Add 'best available' QoS policies #320

Conversation

jacobperron commented Apr 15, 2022 • edited Loading

jacobperron commented Apr 15, 2022 • edited Loading

wjwwood left a comment

Choose a reason for hiding this comment

ivanpauno left a comment

Choose a reason for hiding this comment

jacobperron commented Apr 19, 2022

jacobperron commented Apr 19, 2022

jacobperron commented Apr 19, 2022

jacobperron commented Apr 19, 2022

ivanpauno left a comment

Choose a reason for hiding this comment

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood left a comment

Choose a reason for hiding this comment

jacobperron commented Apr 26, 2022

jacobperron commented Apr 26, 2022

j-rivero commented Apr 29, 2022

jacobperron commented Apr 15, 2022 •

edited

Loading

jacobperron commented Apr 15, 2022 •

edited

Loading