-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
serialize broadcast topology updates and coalesce updates #117
Conversation
updates then coalesce into a single update to gossip Fixes #116
updated PR as per review comments. PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Does it work?
local_peer.go
Outdated
} | ||
} | ||
} | ||
|
||
func (peer *localPeer) broadcastPendingTopologyUpdates() { | ||
peer.router.Routes.recalculate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this is async, I think it's unwise to move it here.
I'm not entirely sure it was ever necessary, but left at the original place there is some chance it will happen before the broadcast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure it was ever necessary
I changed it so to avoid recalculate an every add/delete connection. But considering #111
I reverted to keep original calls to routes.recalculate().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I need to withdraw my approval as unit tests are failing.
I couldn't see an easy fix.
Fixed the UT. Added |
This PR is an attempt to address high topology updates gossip's that can result in certain use-cases.
Fixes #116