Skip to content

Use fibonacci backoff to allow full wait time for new environment 403s#8

Open
henryrecker-pingidentity wants to merge 1 commit intomainfrom
CDI-675-backoff-retries-to-allow-longer-403-wait
Open

Use fibonacci backoff to allow full wait time for new environment 403s#8
henryrecker-pingidentity wants to merge 1 commit intomainfrom
CDI-675-backoff-retries-to-allow-longer-403-wait

Conversation

@henryrecker-pingidentity
Copy link
Copy Markdown
Contributor

  • Use a backoff function to allow full 30+ second wait time for 403 errors when working with brand-new environments. Previously that max retry limit of 10 would prevent a full wait for 403 errors to resolve
  • Use fibonacci backoff and a smaller max backoff duration of 10 seconds when retrying 403 errors

Copy link
Copy Markdown
Collaborator

@patrickcping patrickcping left a comment

Choose a reason for hiding this comment

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

I'm in agreement that the resources that need it get a smarter backoff strategy. Just need to check whether having two backoff implementations to do roughly the same thing is the right way to go

} No newline at end of file
}

func calculateFibonacciBackoff(attempt int, baseDelay time.Duration, maxDuration time.Duration) (time.Duration, error) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there a material difference between the two backoff strategies? Should we just flatten to a single algorithm?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I felt that exponential was excessive given our previous 403 retries were just running every second, and fibonacci is a good middle ground that still actually backs off (to a lesser extent).

I actually do think that fibonacci makes more sense than exponential for our purposes, and I think a 30 second max backoff is probably excessive in most cases. I just was trying to not modify existing functionality in this PR. If you're happy with removing exponential then I can include that in this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok I took a look at the P1 docs and they do explicitly recommend exponential backoff for retries, so maybe it's best that we stick with them https://developer.pingidentity.com/pingone-api/platform/working-with-pingone-apis/retry-best-practices.html

Do you remember if there was a reason you went with retries every second originally for these 403 errors vs. the exponential backoffs we use for other retries?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think, at the time, it was just to keep the implementation simple while we were waiting for more guidance

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

Successfully merging this pull request may close these issues.

2 participants