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

JitterBackoff does not implement Reset() #1

Open
oryband opened this issue Mar 24, 2015 · 0 comments
Open

JitterBackoff does not implement Reset() #1

oryband opened this issue Mar 24, 2015 · 0 comments

Comments

@oryband
Copy link

oryband commented Mar 24, 2015

First of all, great project! It's a much simpler way of doing things compared to https://github.com/eapache/go-resiliency. 👍

I'm getting this compilation error when using JitterBackoff:

cannot use b (type *goback.JitterBackoff) as type goback.Backoff in argument to goback.Wait: *goback.JitterBackoff does not implement goback.Backoff (missing Reset method)

This is because you didn't subclass JitterBackoff from SimpleBackoff (i.e. embedded it inside), but just defined it as a type alias. However, this is not really an alias, and go considers this as a new type. Because of this, this does not make the original Reset() apply to JitterBackoff as well. Thus, go complains that no Reset() implementation exists for the new JitterBackoff type.

Since the only difference I see between these two structs is a single line in NextAttempt() (the one that adds jitter), my best solution to this is to refactor your code a bit:

  1. Make a single Backoff type, no need for two simple and jitter types.
  2. Add to it an extra jitter bool member.
  3. Create a NewSimpleBackoff() and NewJitterBackoff() functions. Where NewSimpleBackoff() returns a new Backoff instance with jitter set to false. NewJitterBackoff would set it to true.
  4. Implement NextAttempt() only once, and check for jitter's value to decide whether to add jitter or not.

Pros for this:

  1. Using NewX() init functions, which is a go idiom for creating instances, and is strict about the arguments it receives, being able to check for valid function arguments. Asking for the user to just init a new struct themselves (e.g. b := &goback.JitterBackof{..}) is dangerous. It took me 15 minutes just to scan your source and make sure I'm setting all values right, and understand that I shouldn't touch the public Attempts member.
  2. Cut back on lines of code, and a simple code structure - you only have a single type (Backoff) and not two (Simple, Jitter).
  3. You get to fix this bug I've originally opened this issue for. :)

Keep up the good work!

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

No branches or pull requests

1 participant