feat: implement suspected#22
Conversation
| incarnation int | ||
| timeout *time.Timer | ||
| gossipPort string | ||
| mu sync.Mutex |
There was a problem hiding this comment.
We should probably use RWMutex instead of mutex.
There was a problem hiding this comment.
I looked into this and found out that RWMutex is pretty complex internally and might actually slow things down. Probably not worth switching.
pkg/node/gossip_handlers.go
Outdated
| payload := node.removeGossip() | ||
| message := gossip.NewMessage( | ||
| gossip.PingReq, | ||
| id, |
There was a problem hiding this comment.
shouldn't the subject be targetPeer here?
pkg/peer/peer.go
Outdated
| Dead PeerStatus = "dead" | ||
| ) | ||
|
|
||
| func (leftPeer Peer) IsGreater(rightPeer Peer) bool { |
There was a problem hiding this comment.
Can we rename this to Supersedes?
pkg/peer/peer.go
Outdated
| return false | ||
| } | ||
|
|
||
| func (leftStatus PeerStatus) IsGreater(rightStatus PeerStatus) bool { |
There was a problem hiding this comment.
I don't think this is ever used outside of peer can we rename it to supersedes (private).
pkg/node/gossip_handlers.go
Outdated
|
|
||
| func (n *Node) handleDeadStatus(id string, updatedPeer peer.Peer) { | ||
| // drop payloads about yourself | ||
| if id == n.id { |
There was a problem hiding this comment.
Reminder for us: This is a potential bug that's being covered up, we should look into other parts of the codebase.
pkg/node/node.go
Outdated
| for newId, newPeer := range newMsg.Peers { | ||
| if newId == oldId { | ||
| if newPeer.IsGreater(oldPeer) { | ||
| updatedPeers[oldId] = newPeer |
There was a problem hiding this comment.
I think we can use the id for newMsg to access the peer in oldMsg. Also I think it's better to update the oldPeer in place instead of creating a new queue.
func (n *Node) addGossip(newMsg *gossip.MessagePayload) {
// nil gaurd
for _, oldMsg := range n.gossipQueue {
// nil gaurd
// replace stale updates about peers in old message
for id, newPeer := range newMsg.Peers {
if oldPeer, ok := oldMsg.Peers[id]; ok {
if newPeer.IsGreater(oldPeer) {
oldMsg.Peers[id] = newPeer
}
delete(newMsg.Peers, id)
}
}
}
// logic for dropping gossip if capacity hit untouched
}
hrotovb001
force-pushed
the
feat/suspected
branch
from
2bf6f98 to
fe50f95
Compare
pkg/node/gossip_handlers.go
Outdated
| }, | ||
| } | ||
| payload := gossip.NewPayload(peers, true) | ||
| n.addGossip(payload) |
There was a problem hiding this comment.
Maybe we just send the refutation immediately? I think refuting immediately resolves the uncertainty about the comment on line 186 of this file.
|
This LGTM thanks @hrotovb001. We can probably fix anything else we missed in a separate PR so we don't keep working on the same diff. |
2 participants
Fixes #11