Handle 'best_available' QoS policies #598
Conversation
If users set certain QoS policies to BEST_AVAILABLE, then we query the graph for endpoint info. The policy is updated such that it is compatible with all endpoints while maintaining the highest possible level of service. Signed-off-by: Jacob Perron <[email protected]>
|
I see a couple of functions |
|
Good point about services and clients. I'm not sure if we should support matching for them or not. At the very least we should define what happens if we provide 'best effort' policies. We have explicit policies for service and clients, and I think it is rare, and perhaps not recommended, to create services and clients that are not using these policies. So, I'm not sure if we should try handling 'best effort' policies for services. I need to think about it a little more. |
|
A final though about design: since we are adding almost the same code in every rmw_qos_profile_t adapted_qos_policies = *qos_policies;
rmw_ret_t ret = rmw_dds_common::qos_profile_get_best_available_for_topic_publisher(
node, topic_name, &adapted_qos_policies, rmw_get_subscriptions_info_by_topic);
if (RMW_RET_OK != ret) {
return nullptr;
}Would it make sense to move it to a different layer of the stack ( |
There was a problem hiding this comment.
lgtm
I think the boilerplate that is there is not likely to change, it's essentially just calling the common function. So I wouldn't try to de-duplicate it I think, because if we did it would probably involve a macro and I don't think that's a a good idea personally. But maybe a c++ function that returns the adapted qos policy might shorten it slightly.
|
I'm not sure how much more of the boilerplate we could move into a common place (e.g. rmw_dds_common). Like William points out, we could try to change the signature of std::optional<rmw_qos_profile_t adapted_qos_policies =
rmw_dds_common::qos_profile_get_best_available_for_topic_publisher(
node, topic_name, qos_policies, rmw_get_subscriptions_info_by_topic);
if (!adapted_qos_policies) {
return nullptr;
} |
Signed-off-by: Jacob Perron <[email protected]>
|
I'm proposing we "ignore" the best available policy for services by defaulting to values in |
|
I see some failures in my local workspace related to fastrtps_dynamic. Not sure if they are related to my local setup: We can wait for Jenkins valid results before investing time in inspecting the errors. |
|
CI is green: ros2/rmw#320 (comment) |
See #599 |
This reverts commit ee60ba6.
Connects to ros2/rmw#320
If users set certain QoS policies to BEST_AVAILABLE, then we query the graph for endpoint info.
The policy is updated such that it is compatible with all endpoints while maintaining the
highest possible level of service.