Skip to content

More flexible refactor. - #6

Merged
ammario merged 7 commits into
developfrom
refactor-proposal
Jan 24, 2018
Merged

More flexible refactor.#6
ammario merged 7 commits into
developfrom
refactor-proposal

Conversation

@ammario

@ammario ammario commented Jan 24, 2018

Copy link
Copy Markdown
Member

No description provided.

@ammario
ammario requested a review from nhooyr January 24, 2018 20:13
Comment thread retry.go Outdated
func OnErrors(errs ...error) func(err error) bool {
return func(err error) bool {
for _, checkErr := range errs {
if err == checkErr {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we errors.Cause these?

@ammario
ammario requested a review from nytopop January 24, 2018 20:29
Comment thread retry.go Outdated
if !cond(err) {
return err
func (r *Retry) postCheck(err error) bool {
if err == nil {

@nytopop nytopop Jan 24, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you think about removing this err check, or only doing it if len(r.postConditions) == 0? If we did that then we can use retry to hammer something until it fails hard (in tests?). OnErrors would filter out anything not explicitly permitted in any case, if supplied as a condition.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So you're saying we want to support a case where a retry continues if the error is nil?

I will definitely turn this check into a proper postCondition.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep. By default though, with no postConditions we should still continue until err == nil, I think.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Such a strange caller is strange enough to also declare an errContinue type

@nytopop nytopop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, when this is merged we should bump semver.

@ammario

ammario commented Jan 24, 2018

Copy link
Copy Markdown
Member Author

@nytopop plz review Jitter method.

Comment thread retry.go Outdated
// Jitter adds some random jitter to the retry's sleep.
//
// It will multiply the sleep by a random between min and max.
func (r *Retry) Jitter(min, max float64) *Retry {

@nytopop nytopop Jan 24, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could use a single relative percent/ratio here for simplicity. Then something like rand.Float64() * ratio and randomly add or remove that multiplier from the current timeout.

someretry.Jitter(0.25) // adds +/- 0-25% jitter
@ammario
ammario requested a review from nytopop January 24, 2018 21:46
@ammario
ammario merged commit f0d1177 into develop Jan 24, 2018
@ammario
ammario deleted the refactor-proposal branch January 24, 2018 22:05
Comment thread retry.go
underlyingSleep := r.sleepDur
r.sleepDur = func() time.Duration {
dur := underlyingSleep()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

doesn't seem to work :\

This should, I believe

mul := rnd.Float64() * rat
if rnd.Intn(2) == 1 {
    mul *= (-1)
}
dur = time.Duration(float64(dur) + (float64(dur) * mul))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's working in the tests?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oops nvm

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Inserting fmt printlns shows that the jitter is always identical

ratio: 
original: 
jittered: 

0.9999
1ms
999.999µs

0.9999
1ms
999.999µs

0.9999
1ms
999.999µs

0.9999
1ms
999.999µs

0.9999
1ms
999.999µs

0.9999
1ms
999.999µs

0.9999
1ms
999.999µs

0.9999
1ms
999.999µs

0.9999
1ms
999.999µs

0.9999
1ms
999.999µs

0.9999
1ms
999.999µs

0.9999
1ms
999.999µs

0.9999
1ms
999.999µs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants