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

[MM-43508] Implement basic reconnection support for rtcd client #59

Merged
merged 3 commits into from
May 6, 2022

Conversation

streamer45
Copy link
Collaborator

Summary

PR adds reconnection support for the rtcd client as implemented in mattermost/rtcd#24

  • If a disconnection occurs, we first attempt to re-register the client in case the rtcd instance restarted and wiped its store.
  • Whether that fails or not we keep trying up to maxReconnectAttempts (default to 8) with some incremental backoff (2 seconds).
  • If maxReconnectAttempts is reached we give up and disable the plugin.

To note, this doesn't cover initial connection on plugin startup. Adding that support makes the logic even more complex so I opted to keep the assumption that a running rtcd should be available when enabling the plugin.

Also, this whole logic is going to change a bit when implementing MM-43872 as we are going to deal with multiple concurrent clients but didn't want to couple the two pieces of work so take this as a first iteration.

Ticket Link

https://mattermost.atlassian.net/browse/MM-43508

@streamer45 streamer45 added the 2: Dev Review Requires review by a core committer label May 4, 2022
@streamer45 streamer45 requested a review from cpoile May 4, 2022 08:53
@streamer45 streamer45 self-assigned this May 4, 2022
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

go.mod Outdated
@@ -15,7 +15,7 @@ require (
require (
github.com/mattermost/logr/v2 v2.0.15
github.com/mattermost/mattermost-plugin-api v0.0.27
github.com/mattermost/rtcd v0.3.1-0.20220427165117-035048fda047
github.com/mattermost/rtcd v0.3.1-0.20220504082845-a64f36fa09dc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this will have to be updated once it's merged (you probably know) -- I was stung once by a branch disappearing on me once. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll probably push a new tag since there are significant changes upstream.

@streamer45 streamer45 added 3: Reviews Complete All reviewers have approved the pull request Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) and removed 2: Dev Review Requires review by a core committer labels May 5, 2022
@streamer45 streamer45 removed the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label May 6, 2022
@streamer45 streamer45 merged commit e199fe6 into main May 6, 2022
@streamer45 streamer45 deleted the MM-43508 branch May 6, 2022 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants