feat: behaviour penalty when non-priority queue reaches maxNumElementsInNonPriorityQueue #1083
Conversation
diegomrsantos
force-pushed
the
penalty-non-priority-queue
branch
from
b21e90f to
a6237bd
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1083 +/- ##
============================================
- Coverage 84.82% 84.60% -0.23%
============================================
Files 91 91
Lines 15428 15486 +58
============================================
+ Hits 13087 13102 +15
- Misses 2341 2384 +43
|
|
This log shows the moment when the peer 16U*23VggQ non-prio queue reaches |
| p.closeSendConn(PubSubPeerEventKind.DisconnectionRequested) | ||
| else: | ||
| Future[void].completed() | ||
| p.behaviourPenalty += 0.0001 |
There was a problem hiding this comment.
I'd suggest moving the value used here to a constant.
| p.behaviourPenalty += 0.0001 | ||
| trace "Peer has reached maxNumElementsInNonPriorityQueue. Discarding message and applying behaviour penalty.", peer = p, score = p.score, | ||
| behaviourPenalty = p.behaviourPenalty, agent = p.getAgent() | ||
| Future[void].completed() |
There was a problem hiding this comment.
Given most return paths are Future[void].completed() how about moving it outside the if statement and doing an explicit return on each arm for the cases that are not Future[void].completed().
There was a problem hiding this comment.
could you please elaborate on what you believe is bad with the current approach?
There was a problem hiding this comment.
There's nothing wrong per se. What I was hinting at was, when you have code akin to this:
if <cond>:
foo()
return A
elif <cond>:
bar()
return A
elif <cond>:
baz()
return B
...
else:
foobar()
return ASometimes it's clearer to group/extract the returns:
if <cond>:
foo()
elif <cond>:
bar()
elif <cond>:
baz()
return B
...
else:
foobar()
return ABut I just realised here it's almost 50/50, so not a lot of improvement can be made.
| proc clearNonPriorityQueue*(p: PubSubPeer) = | ||
| if len(p.rpcmessagequeue.nonPriorityQueue) > 0: | ||
| p.rpcmessagequeue.nonPriorityQueue.clear() | ||
|
|
||
| when defined(pubsubpeer_queue_metrics): | ||
| libp2p_gossipsub_non_priority_queue_size.set(labelValues = [$p.peerId], value = 0) | ||
|
|
There was a problem hiding this comment.
How asynchronous is this operation? That is: How feasible it is that, between the clear call and the metrics update, some value is added to the nonPriorityQueue? 🤔
There was a problem hiding this comment.
This proc isn't async and won't be suspended.
There was a problem hiding this comment.
Oh, that's right. nvm then
This PR applies a behavior penalty to peers whose non-prio queue reaches the max limit configured, instead of the previous strategy of disconnecting the peer. A conservative penalty of
0.0001is added to behaviourPenalty for each message tried to be sent when the queue is over the limit, and the message is discarded. This usually results in abehaviourPenaltyaround [0.1, 0.2] when the score is updated and its value around [-0.4, -0.1].It causes the peer to be pruned due to its negative score. This PR also clears the non-prio queue at this moment.