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 backoff #2

Merged
merged 3 commits into from Aug 9, 2018

Conversation

Projects
None yet
2 participants
@talal
Copy link
Contributor

talal commented Aug 8, 2018

Any suggestions?

@talal talal requested a review from majewsky Aug 8, 2018

@talal talal force-pushed the backoff branch from a5731e3 to 955f970 Aug 8, 2018

@majewsky

This comment has been minimized.

Copy link
Contributor

majewsky commented Aug 8, 2018

Some small things first:

  • x and y should be replaced by more speaking names.
  • x is not a time.Duration, it's an integer factor.

As a general note, when you have a function that takes another function, I like to place the function argument at the end of the argument list. When the function argument is a long anonymous function, the other arguments otherwise get separated from the function call:

err := doSomething(function() error {
  if aLotOfThings.Happen() {
    in.ThisFunction(AndMaybe {
       There: "are",
       More: "braces",
       And: "stuff",
    })
  }
  then(Lines(100).DownBelow())
  itIsNotClear = true
}, 2, 4) //...which function call these arguments belong to

vs.

err := doSomething(2, 4, function() error {
  ...
  ...
  ...
})

Finally, I would suggest a different API design altogether. The problem with function arguments is that it's sometimes difficult to tell from context what the arguments mean. An extreme example (which I sadly cannot find on Google right now) is the Win32 API function for starting a new process which takes 22 arguments and it looks something like:

StartProcess("C:\Windows\system32\rundll.exe", null, null, null, null, null, null, null, null, true, true, false, false, null, false, true);

So unless you have the API docs open on your other monitor, it's pretty impossible to tell what each of these arguments mean. To be able to give meaningful names to arguments, it's common practice in Go APIs to collect all these switches and configuration options into an Options struct:

type RetryOptions struct {
  BackoffFactor int
  MaxInterval time.Duration
}
func Retry(opts RetryOptions, action func() error) { ... }

//usage example:
Retry(RetryOptions { BackoffFactor: 2, MaxInterval: 5 * time.Second }, func() {
  ...
})

That's much more verbose, but also much more obvious when you're reading it. It also has the advantage that you can later add new fields to RetryOptions without breaking existing users of your API. (If you look at the API of https://godoc.org/github.com/majewsky/schwift, you'll see these Options types all over the place for this reason.)

Finally, here's the API that I would propose:

type RetryStrategy interface {
  RetryUntilSuccessful(action func() error)
}

type ExponentialBackoff struct {
  Factor int
  MaxInterval time.Duration
}
func (eb ExponentialBackoff) RetryUntilSuccessful(action func() error) { ... }

//usage example:
ExponentialBackoff {
  Factor: 2,
  MaxInterval: 5 * time.Second,
}.RetryUntilSuccessful(func() error {
  ...
})

This makes it easy to later add other implementations for type RetryStrategy (e.g. one that just retries for a given number of times and then gives up). It also allows other parts of the program to take a RetryStrategy as a parameter.

@talal

This comment has been minimized.

Copy link
Contributor Author

talal commented Aug 8, 2018

updated as suggested.

@majewsky majewsky merged commit 9ed5250 into master Aug 9, 2018

@majewsky majewsky deleted the backoff branch Aug 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.