-
Notifications
You must be signed in to change notification settings - Fork 993
feat(discovery): add jitter and fix backoff #2967
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
base: main
Are you sure you want to change the base?
Conversation
nodersteam
commented
Nov 27, 2023
- Replacing time.NewTicker with time.NewTimer
- Adding backoff logic
- Adding jitter to backoff
- Removed unused test variable
…er, adding a backoffDuration, adding a little jitter
Is it acceptable to use math/rand or add generation logic via crypto/rand to fix Lint? |
@Wondertan Could you please check this PR? |
Hey @nodersteam, I am deferring the review of this to @walldiss, as the most prominent expert on that code. |
Got it! Thx |
"time" | ||
|
||
"golang.org/x/sync/errgroup" |
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.
Better to keep as it was.
discoveryRetryTimeout = time.Millisecond * 100 // defined in discovery.go | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), time.Second*20) | ||
ctx, cancel := context.WithTimeout(context.Background(), time.Minute*10) |
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.
Why so big? 30x more than before 👀
continue | ||
} | ||
d.backoffDuration = backoffInitialDuration |
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.
Should we also reset backoffTimer
here?