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

removed header logging. added jitter #87

Merged
merged 2 commits into from
Apr 18, 2023

Conversation

sbSteveK
Copy link
Contributor

  • add jitter
  • remove header logging.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@xiazhvera xiazhvera left a comment

Choose a reason for hiding this comment

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

Other than the comments, the PR looks good to me.


static uint64_t s_aws_secure_tunnel_compute_reconnect_backoff_full_jitter(struct aws_secure_tunnel *secure_tunnel) {
uint64_t non_jittered = s_aws_secure_tunnel_compute_reconnect_backoff_no_jitter(secure_tunnel);
return aws_secure_tunnel_random_in_range(0, non_jittered);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the range be [MIN_RECONNECT_DELAY_MS, non_jittered] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think full jitter works in the entire range between 0 and max sleep. An argument could be made to use min but unless there's a good reason to, I think it just reduces the range of available times on a large scale reconnection event.

@sbSteveK sbSteveK merged commit cabba56 into secure-tunnel-v3 Apr 18, 2023
@sbSteveK sbSteveK deleted the secure-tunnel-v3-jitter branch April 18, 2023 16:29
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.

2 participants