Skip to content
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

Enable CongestionControl::Block if QoS is reliable. #43

Open
wants to merge 3 commits into
base: dev/1.0.0
Choose a base branch
from

Conversation

evshary
Copy link

@evshary evshary commented Nov 22, 2024

With DDS the only combination of QoS which makes a Publisher to block the publications in case of network congestion is with Reliability=RELIABLE and History=KEEP_ALL.
In order to closely mimic this behaviour rmw_zenoh is setting Z_CONGESTION_CONTROL_BLOCK only for this combinaison of QoS.

However, in case of a Publisher for a "latched topic", it will use such combination of QoS:
Reliability=RELIABLE, History=KEEP_LAST(1) and Durability=TRANSIENT_LOCAL.
This leads rmw_zenoh to set Z_CONGESTION_CONTROL_DROP for the Publisher. And in case of very congested network the single publication might be dropped. That's what happens in the case discussed here:
#38 (comment)

This PR solves this problem setting Z_CONGESTION_CONTROL_BLOCK for any Publisher with QoS Reliability=RELIABLE, whatever the other QoS are.

@evshary evshary requested a review from JEnoch November 22, 2024 10:13
@JEnoch
Copy link

JEnoch commented Nov 22, 2024

This looks good to me.
@clalancette could you please also review it and advise if we shall merge it to dev/1.0.0 branch now or if we shall wait the merge of dev/1.0.0 to rolling

@JEnoch JEnoch marked this pull request as draft November 22, 2024 11:34
@evshary
Copy link
Author

evshary commented Nov 22, 2024

@JEnoch Thank you for adding comments and a detailed description. I always admire the ability you make things clearer 😆
I will also test the PR with nav2 to verify it again.

@evshary
Copy link
Author

evshary commented Nov 25, 2024

I will also test the PR with nav2 to verify it again.

Okay, it works properly after the changes.

@evshary evshary mentioned this pull request Nov 25, 2024
@JEnoch JEnoch marked this pull request as ready for review November 25, 2024 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants