-
Notifications
You must be signed in to change notification settings - Fork 551
Set subscriber depth to max from publishers #1020
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.
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
base: ros2
Are you sure you want to change the base?
Conversation
I don't buy this premise. From my understanding, subscriber will lose messages if the queue gets filled faster than the executor is able to process callbacks. This can happen if the publisher outputs messages with too high frequency (in which case setting higher depth will only delay the loss of messages) or the publisher outputs the messages in short bursts (in which case increasing subscriber depth will help). I think both situations can happen even if publisher uses lower depth QoS then subscriber. I might be wrong though. Curious about your specific use-case. |
You are correct that it is not enough that the publisher has a bigger queue, it is necessary that it is actually used, that is, that more than 10 messages are sent "simultaneously" (or faster than the subscriber can process them). When sending quick bursts of messages, having a longer queue on the publisher side will allow more messages to be sent "simultaneously", which will fill up the receiving queue faster, regardless of how quick the processing is, leadng to message loss (which can be solved by increasing the subscriber depth). My specific use case is:
In that case the 100 messages fill the outgoing queue, get sent "simultaneously" (or close enough), and quickly fill the |
Have you tried setting
I guess we could treat the publisher's queue size as the declaration of how many messages in a bulk can it send but I don't see it as a golden rule. For example, what if there are multiple publishers? @sea-bass Do you have any opinion here? |
Setting
In the case of multiple publishers, the proposed code would simply set the queue to the maximum length between all of them, which will be enough to hold whichever burst is sent from any of the publishers. For the other publishers, which don't use that many messages, the rest of the queue will be unused. In essence, the queue size of the publisher is the maximum ammount of messages that can be sent in a burst (as any additional messages will be lost by the outgoing queue), though it is true that there is no need to actually fill all the queue. If less messages than that number are sent in a sigle burst, the queue simply isn't filled, which doesn't really cause any issues. |
Interesting. It is true that the
At the same time, setting it to max probably has memory usage implications for users that do not need such high traffic of messages. So maybe this should be a configurable launch parameter that defaults to |
Actually another idea to control this at the per-topic level... is the issue here also related to the fact that the Would adding one there help in this case? |
Yes, though I don't think the implications should be that bad, as it will only matter if the queue actually gets filled (wither too many messages too fast, or too big a burst), and in both cases you lose the messages otherwise.
I don't think putting the queue size as a launch parameter is a good idea, as it would set the same queue for all topics, regardless of the need of that, while setting it to "max of publishers" sets that in a per-topic basis. However, having a
No, the |
@davidferlop Would extending the protocol to be able to set subscriber depth from client side (for example, using the |
Yes, that would work as well. |
Public API Changes
None
Description
Set the max depth of a subscriber to the maximum max depth of all publishers. Currently, if a publisher has a depth higher than 10 the messages will be lost.