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

Add randomizing servers in connect #35

Merged
merged 2 commits into from
Feb 20, 2024
Merged

Add randomizing servers in connect #35

merged 2 commits into from
Feb 20, 2024

Conversation

piotrpio
Copy link
Contributor

No description provided.

@piotrpio piotrpio requested a review from Jarema February 16, 2024 12:52
@piotrpio piotrpio force-pushed the url-randomize branch 2 times, most recently from dfcc313 to ebe4a8b Compare February 16, 2024 12:54
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

Looks good. Just one change request.

@@ -13,9 +13,10 @@ public class ClientOptions {
private var pingInterval: TimeInterval = 60.0
private var reconnectWait: TimeInterval = 2.0
private var maxReconnects: Int = 60
Copy link
Member

Choose a reason for hiding this comment

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

We should not set max reconnects by default, as it's a known footgun. It will be especially painful on mobile devices.

Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
@piotrpio piotrpio requested a review from Jarema February 20, 2024 09:02
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

LGTM!

@piotrpio piotrpio merged commit b9e2cf0 into main Feb 20, 2024
1 check passed
@piotrpio piotrpio deleted the url-randomize branch February 20, 2024 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants