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

Incomplete Retry schedule implementation #299

Open
leonerd opened this issue Nov 5, 2018 · 2 comments
Open

Incomplete Retry schedule implementation #299

leonerd opened this issue Nov 5, 2018 · 2 comments

Comments

@leonerd
Copy link
Contributor

leonerd commented Nov 5, 2018

From looking at the definition of a retry schedule (e.g. from Paws/SMS.pm) it would appear to have three parameters:

{ base => 'rand', type => 'exponential', growth_factor => 2 }
(https://metacpan.org/source/JLMARTIN/Paws-0.38/lib/Paws/SMS.pm#L10)

In fact, the code only obeys the type argument; both the random base and the growth factor of 2 are hardcoded into the algorithm:

(2 ** ($self->tries - 2)) + (rand(1) / 2);
https://metacpan.org/source/JLMARTIN/Paws-0.38/lib/Paws/API/Retry.pm#L41

Almost all of the service definitions hardcode the very same definition for the retry schedule, though a few do not; e.g.

lib/Paws/DynamoDB.pm:10:    { base => '0.05', type => 'exponential', growth_factor => 2 }

so the user may be surprised to find this is not honoured.

@leonerd
Copy link
Contributor Author

leonerd commented Nov 5, 2018

Given as the implementation hardcodes a growth factor of 2, and that is supplied to every single service default anyway, I'd suggest it might be simpler for now just to remove that from the definitions of them, since then nobody gets surprised if they change the value and it isn't honoured.

However, I see that those 2s are template-generated from the service definition JSON files, so it's only correct that currently everything uses a factor of 2. Maybe oneday one of these will change. Perhaps the code should be extended to use that after all?

@pplu
Copy link
Owner

pplu commented Nov 21, 2018

When I implemented the exponential backoff, I first had to import the parameters from botocore, thinking that I would implement the same as them, but I started getting strange results. It turns out that the backoff didn't seem exponentialit would have strange wait times, sometimes finally timing out very fast, and sometimes very slowly). That make me change it to be more "predictable", but I ended up ignoring some of the parameters from boto.

Note: it looks like botocore has the same problems the way they implement the backoff boto/botocore#1471

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

2 participants