-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Some fixes #687
Some fixes #687
Conversation
bea4ff1
to
b88685d
Compare
ffaec73
to
e82992c
Compare
ping @containous/traefik |
// JobBackOff is an exponential backoff implementation for long running jobs. | ||
// In long running jobs, an operation() that fails after a long Duration should not increments the backoff period. | ||
// If operation() takes more than MinJobInterval, Reset() is called in NextBackOff(). | ||
type JobBackOff struct { |
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.
Hum I'm not sure about the naming of the package (and also the import we to). Shouldn't the package be job
(or something like that) and this struct BackOff
as an job implem' using a backoff strategie ?
And thus, the import wouldn't need to be aliased below.
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.
Hum, import alias would still be needed ẁith backoff.RetryNotify
no?
err := backoff.RetryNotify(operation, job.NewJobBackOff(backoff.NewExponentialBackOff()), notify)
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.
no because you're package would be named job
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.
(instead of backoff
currently — which is why you need to do an alias as of this PR)
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.
Indeed, fixed :)
d5a1dec
to
0e88566
Compare
LGTM |
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.
LGTM 🐸
@emilevauge needs a rebase 👼
LGTM on rebase |
0e88566
to
e4d8f54
Compare
e4d8f54
to
a882a9d
Compare
Fixes #451