Conversation
bucket.go
Outdated
| func (b *bucket) min(lessThan func(p1 *PeerInfo, p2 *PeerInfo) bool) *PeerInfo { | ||
| if b.list.Len() == 0 { | ||
| return nil | ||
| func (b *bucket) findFirst(p func(p *PeerInfo) bool) *PeerInfo { |
There was a problem hiding this comment.
I'd still use a comparison function like min that determines which peer is more "evictable" than another peer. Then we go through the bucket and find the most evictable peer and check if it is actually evictable.
Previously: Find oldest time, then check if time is too old
This PR: Find first replaceable
Suggestion: comparison function if p1.replaceable return p1; if p2.replaceable return p2; return p1. Evaluation function return p.replaceable. It's really easy to plug in the older code here if required.
Note: if we wanted to enable stable sorting we would have to compare peerIDs if both results were equal (e.g. added at the same time or both replaceable), but since we just need one of them it shouldn't really matter.
There was a problem hiding this comment.
I've made this change. Let me know what you think. And I agree, we don't need a stable sort for now.
| // MarkAllPeersIrreplaceable marks all peers in the routing table as irreplaceable | ||
| // This means that we will never replace an existing peer in the table to make space for a new peer. | ||
| // However, they can still be removed by calling the `RemovePeer` API. | ||
| func (rt *RoutingTable) MarkAllPeersIrreplaceable() { |
There was a problem hiding this comment.
This feels like a function just waiting to get deprecated, as opposed to basically exposing the updateAllWith function. However, if we're going to refactor this plus the kad-dht repo in the near future to stop these updates from being such a pain I don't really mind.
| // MarkAllPeersIrreplaceable marks all peers in the routing table as irreplaceable | ||
| // This means that we will never replace an existing peer in the table to make space for a new peer. | ||
| // However, they can still be removed by calling the `RemovePeer` API. | ||
| func (rt *RoutingTable) MarkAllPeersIrreplaceable() { |
There was a problem hiding this comment.
I'd like to see how this is used in the DHT PR to ensure we don't run into race conditions
There was a problem hiding this comment.
The PR is up and I think we can keep this for now. Let me know what you think.
bucket_test.go
Outdated
| b.pushFront(&PeerInfo{Id: pid2, replaceable: false}) | ||
| require.Equal(t, pid1, b.findFirst(func(p *PeerInfo) bool { | ||
| return p.replaceable | ||
| // first is till min |
There was a problem hiding this comment.
| // first is till min | |
| // first is still min |
| replaceablePeer := bucket.findFirst(func(p *PeerInfo) bool { | ||
| return p.replaceable | ||
| replaceablePeer := bucket.min(func(p1 *PeerInfo, p2 *PeerInfo) bool { | ||
| return p1.replaceable |
There was a problem hiding this comment.
maybe add a comment here about us not needing a stable sort here just to call it out
|
3 participants
For libp2p/go-libp2p-kad-dht#661.