Issue-213: Optional automatic retry on 5xx

[ewandennis]

Addresses #213:

• Constructor now accepts a numeric retries option which defaults to undefined.
• By default, no retries are attempted to protect existing tested code paths and compatibility
• When enabled, each request will be retried until it returns a non 5xx status or until retries attempts are reached.

[aydrian]

Could you also add a line to the CHANGELOG.md under the "Unreleased" header? Makes it easier for when we do a release.

Otherwise, it LGTM, but maybe could get a second set of eyes. @jasonrhodes, @aymg?

[ewandennis]

Incidentally, I'd also welcome discussion on whether this should even exist in our client libs and if so, whether it should be enabled by default (after a suitable version bump).

[avigoldman]

I like this in the libraries personally. The functionality isn't heavy or complicated and it makes the process of using our service a smoother experience.

[jasonrhodes]

I definitely don't like the idea of turning it on by default ever, for the sake of not raising an unwitting fleet of accidental DDoSers esp if we have a spat of 503s or some other DNS error.

Other than that I agree it's a nice feature… maybe think about capping retries at something sane? Maybe 3-5 with some back off logic?

[avigoldman]

I agree, it doesn't make sense to turn it on by default. Having some back off logic does feel good.

[aydrian]

Any thoughts on @jasonrhodes suggestions, @ewandennis? Do we have this in the php library? Is it handled the same?

[ewandennis]

Yup, I submitted a similar feature to php-sparkpost at the time.

As for safety, I can see a reason to enforce an upper retry limit. Back-off also sounds good in theory but, since we already ask people to retry on 5xx without back-off and the world has not ended, I wonder if it's really necessary. Back-off isn't a very intuitive mechanism for the user either so I wonder if we might get all-new questions about API call latency.

[aydrian]

Awesome. Thank you. @jasonrhodes / @avigoldman Can you take a look? We can merge this in and I'll do a minor release.

[crobinson42]

Why did this die? This is a great feature to have - my Node.js server gets 503 errors randomly from Sparkpost when trying to send and this is starting to become concerning. An automatic retry would be great to have confidence in the service.