-
Notifications
You must be signed in to change notification settings - Fork 94
[Core] New LazyConnection with background auto-reconnect #3215
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
Conversation
6aafcc4
to
1dca721
Compare
2870202
to
2a5c795
Compare
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.
Great work @AhmedSoliman :-) LGTM. +1 for merging.
Err( | ||
// We don't know this node-id yet, but we might in the future, keep retrying. | ||
err @ ConnectError::Discovery(DiscoveryError::UnknownNodeId(_)), | ||
) => { | ||
trace!(%swimlane, %peer, "Cannot to connect, will retry: {}", err); | ||
continue 'connect; |
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.
Could this be a busy loop?
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.
Yes, good catch. I'll add a hardcoded 500ms sleep here.
Other changes: - Smaller EgressMessages by using static string for message names, removing span for unary messages (224 bytes -> 216 bytes) - ReplyRx doesn't require protocol version at creation time, this will become relevant in a subsequent PR. The protocol version is now returned along with the reply when it arrives
Stack created with Sapling. Best reviewed with ReviewStack.