use queue size to ensure history depth #31
Conversation
| if (status != DDS::RETCODE_OK) { | ||
| rmw_set_error_string("failed to get default datawriter qos"); | ||
| // printf("get_default_datawriter_qos() failed. Status = %d\n", status); | ||
| return nullptr; | ||
| } | ||
|
|
||
| // ensure the history depth is at least the requested queue size | ||
| if (datawriter_qos.history.kind == DDS::KEEP_LAST_HISTORY_QOS && |
There was a problem hiding this comment.
Why check if the history kind is correct? Why not always set the history.kind to that and always set the history.depth to queue_size?
There was a problem hiding this comment.
If the user has already configured a "better" history (e.g. keep all or a larger number) then the code should not reduce the history depth.
There was a problem hiding this comment.
When would the user have had the chance to do that? The publisher was just created in this function. At best they could do it after the publisher is returned from this function.
There was a problem hiding this comment.
It is always possible to configure QoS parameters externally, e.g. via XML files.
There was a problem hiding this comment.
My concern is that if the default changes history kind then the queue size is not even set. Also, if I pass a queue size into this create function I would expect the queue is set to exactly what I asked, regardless of the default history settings.
There was a problem hiding this comment.
If we want to be able to fall back to system defaults, then we need a "create default publisher" option. I think it is the wrong behavior for the function to receive a queue_size and then potentially not use it.
There was a problem hiding this comment.
Always setting the history kind and depth here would prevent using vendor specific configuration options completely.
The history kind only support ALL and DEPTH N. If the default has been set to ALL the function doesn't reduce the history. If it is set to a specific DEPTH it ensures that it has at least the requested queue size. But if the default has been changed to a larger value it is not reduced.
Imo if the user changes the default configuration he is doing that for a specific reason and I would not overwrite that choice in our code.
There was a problem hiding this comment.
The ability to do that is an implementation detail, this function must behave consistently based on the input. I think that if I design my node to explicitly need a queue size of 1 and you set the global to keep all that will not work. If you want to use the defaults then we need a way to specify "use default", which is how it is modeled in DDS. We cannot ask users to always specify a queue_size and then potentially ignore it.
There was a problem hiding this comment.
I have created ros2/rmw#13 to possibly extend the rmw interface to support both cases. Until then we will keep the behavior implemented in this PR.
|
+1 |
1 similar comment
|
+1 |
use queue size to ensure history depth
Connects to ros2/ros2#42
@esteve @tfoote @wjwwood Please review.