Add randomization in sync retry batch peer selection #5822
Conversation
| *peer, | ||
| ) | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
| // Sort peers prioritizing unrelated peers with less active requests. | ||
| priorized_peers.sort_unstable(); | ||
| priorized_peers.first().map(|&(_, _, peer)| peer) | ||
| priorized_peers.first().map(|&(_, _, _, peer)| peer) |
There was a problem hiding this comment.
not related with this PR, but do we need to iterate, collect, and then sort? To get the lowest peer by that Ord criteria we can just iterate once and get the lowest with :
let new_peer = self
.network_globals
.peers
.read()
.synced_peers()
.map(|peer| {
(
failed_peers.contains(peer),
self.active_requests.get(peer).map(|v| v.len()).unwrap_or(0),
rand::thread_rng().gen::<u32>(),
*peer,
)
})
.min()
.map(|(_, _, _, peer)| peer);same applies to the other change on this PR
There was a problem hiding this comment.
Nice suggestion! Pushed
Co-authored-by: João Oliveira <hello@jxs.pt>
|
@Mergifyio queue |
🛑 The pull request has been removed from the queue
|
|
@mergify refresh |
✅ Pull request refreshed |
|
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 2e2ccec |
Issue Addressed
When retrying a batch, peers are prioritized by:
While the prioritization is okay, breaking ties by NodeID is not fair and prioritizes peers that just happen to have a peer ID if a smaller value or those that have farmed peer IDs with smaller values.
Proposed Changes
Add a random u32 before sorting batch retry peers