Add Queue Factory function to support injecting router schedulers#6262
Add Queue Factory function to support injecting router schedulers#6262tychoish merged 7 commits intobez/p2p-revise-queue-scheduler-pqfrom
Conversation
ec83049 to
7629913
Compare
alexanderbez
left a comment
There was a problem hiding this comment.
Looks great @tychoish :)
| peerMtx sync.RWMutex | ||
| peerQueues map[NodeID]queue // outbound messages per peer for all channels | ||
| queueFactory func(int) queue | ||
| queueBufferDefault int |
There was a problem hiding this comment.
Any reason to have this be part of the type instead of just a private constant?
p2p/router.go
Outdated
| queue := newPQScheduler(r.logger, peerID, r.metrics, r.chDescs, 0, 0, defaultCapacity) | ||
| queue.start() | ||
| var peerQueue queue | ||
| func() { |
There was a problem hiding this comment.
Why put this into a function?
There was a problem hiding this comment.
to be able to use the defer on the unlock.
There was a problem hiding this comment.
we can avoid the defer by calling r.peerMtx.Unlock() directly after the code block. Note a blocker, I just find inline functions a bit harder to read.
There was a problem hiding this comment.
I moved this into it's own function, which I think improves readability. Given that the changes means that we have to call a function in the lock, I'm a bit afraid of doing that without a defer, as a matter of practice.
This just centralizes how we construct queue instances and avoids using the fifo queue for ingestion and a different queue for outgoing messages.
It presently defaults to FIFO unless its explicitly overridden, but we may want to use the priority algorithm.