Skip to content
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

Destroy idle node-to-node channels automatically #2672

Closed
jumaffre opened this issue Jun 15, 2021 · 2 comments · Fixed by #5266
Closed

Destroy idle node-to-node channels automatically #2672

jumaffre opened this issue Jun 15, 2021 · 2 comments · Fixed by #5266
Assignees
Milestone

Comments

@jumaffre
Copy link
Contributor

Our current policy to closing node-to-node channels was originally too strict: a channel to a node is destroyed as soon as we have (globally) committed its retirement. #2654 relaxes that, at the cost of keeping channels open forever, which causes a relatively small memory growth (nodes should be recycled frequently enough that this doesn't cause any issues in practice).

There isn't a clear point in time when a node-to-channel can safely be destroyed. For example, as a retired primary, I may want to keep a channel open to other nodes to forward client requests to the new configuration (see #1713). As a retired backup, I may also receive the response of a forwarded RPC from the primary, which should still be returned to the client.

Instead, we should periodically destroy channels that have been idle (both on send and receive) for a while (frequency TBC. but probably a multiple of the election timeout). If a node wants to send a message to a channel that has been destroyed, the message will be queued (*) and a new channel establishment will start. On message reception from a node whose channel was previously destroyed, we simply recreate a new channel.

While the channel re-establishment has already been implemented in #2092, we'll need to periodically tick() the ChannelManager and destroy channels that have been idle since for a while.

(*) To cap memory growth, we only queue one message for now.

@achamayou
Copy link
Member

@eddyashton was this done in #2801 or is it still outstanding?

@eddyashton
Copy link
Member

This is still outstanding.

@achamayou achamayou added the 3.x label May 23, 2022
@shokouedamsr shokouedamsr added this to the 3.x milestone Jul 25, 2022
@achamayou achamayou added 4.x and removed 3.x labels Nov 16, 2022
@shokouedamsr shokouedamsr modified the milestones: 3.x, 4.x Dec 5, 2022
@shokouedamsr shokouedamsr added p1 and removed p2 labels Dec 5, 2022
@eddyashton eddyashton self-assigned this May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants