-
Notifications
You must be signed in to change notification settings - Fork 54
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
Send priority with queue fix #1051
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1051 +/- ##
============================================
+ Coverage 82.77% 82.80% +0.02%
============================================
Files 91 91
Lines 15666 15749 +83
============================================
+ Hits 12968 13041 +73
- Misses 2698 2708 +10
|
discard p.rpcmessagequeue.sendPriorityQueue.popLast() | ||
|
||
when defined(libp2p_expensive_metrics): | ||
libp2p_gossipsub_priority_queue_size.set(labelValues = [$p.peerId]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesnt compile when the flag is enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
26a37e3 - might be a good idea to enable these flags in CI builds
This reapplies #1015 and is a smaller version of #1028 that focuses on clearing the priority queue at both ends since the completion order of send futures may be reversed.
The PR also introduces a workaround for redundant msg copies when sending encoded messages and ensures that
sendMsg
always returns a future that can be used for backpressure.It creates two new metrics
libp2p_gossipsub_priority_queue_size
andlibp2p_gossipsub_non_priority_queue_size
that can be enabled with thepubsubpeer_queue_metrics
flag. It should be used only locally as it is by peer and create a large amount of data.