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

Update client retry delay when calling out to applens endpoint #2780

Merged
merged 2 commits into from
Mar 20, 2023

Conversation

cloudfit-nick
Copy link
Contributor

@cloudfit-nick cloudfit-nick commented Mar 17, 2023

This updates the retry delay from 10 seconds to 3 seconds to ensure that all their calls if needed happen with in 60 seconds.

ARO-2567

weinong
weinong previously approved these changes Mar 17, 2023
Copy link
Contributor

@weinong weinong left a comment

Choose a reason for hiding this comment

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

LGTM. However, we should probably create a common retry policy to be reused. cc: @nwnt

Copy link
Collaborator

@SudoBrendan SudoBrendan left a comment

Choose a reason for hiding this comment

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

This seems fine, is there a work item or bug linked to this? What's the significance of 60 seconds?

@weinong
Copy link
Contributor

weinong commented Mar 17, 2023

This seems fine, is there a work item or bug linked to this? What's the significance of 60 seconds?

All RP frontend apis have 60s budget before ARM times out

Comment on lines 28 to 29
MaxRetries: 3,
RetryDelay: time.Second * 10,
RetryDelay: time.Second * 3,
Copy link
Member

Choose a reason for hiding this comment

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

3 retries times 10 seconds.. that's 30 seconds. What's the timeout per try?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has an exponential back off, with a setting of 3 seconds it was erroring out in ~30 seconds (3 seconds + 12 seconds + round trip / response time from all 3 calls). With a setting of 4 seconds, it was erroring out around 50 seconds.

To be safe I went with a setting of 3 seconds.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense, but isn't immediately obvious. Can we add some code comments here as breadcrumbs for the next person that comes across this particular setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note on the place where it defined the setting it already calls out that its exponential.

@cloudfit-nick
Copy link
Contributor Author

This seems fine, is there a work item or bug linked to this? What's the significance of 60 seconds?

https://issues.redhat.com/browse/ARO-2567

Copy link
Member

@petrkotas petrkotas left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@petrkotas petrkotas merged commit e007f1e into master Mar 20, 2023
@cloudfit-nick cloudfit-nick deleted the nickhundstad-applens-retry-delay branch April 27, 2023 15:44
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.

5 participants