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

Retry more flaps calls and classify errors better #3671

Closed
wants to merge 4 commits into from

Conversation

billyb2
Copy link
Contributor

@billyb2 billyb2 commented Jun 25, 2024

Change Summary

What and Why:
Sorry, this is a two part PR:

  1. We're now retrying every flaps call where it seems relevant. I noticed in tracing that some builder creations were failing unnecessarily
  2. We're now catching more errors. For whatever reason, flaps can return "App not found" or "Could not find app" when it has an issue finding an app. We're doing string comparisons :vomit: to find both

How:
I created a new function that retries flaps errors that are > 500 and < 600. There are likely other errors we can retry (408, for example)

Related to:


Documentation

  • Fresh Produce
  • In superfly/docs, or asked for help from docs team
  • n/a

Sorry, this is a two part PR:
1. We're now retrying every flaps call where it seems relevant. I
noticed in tracing that some builder creations were failing
unnecessarily
2. We're now catching more errors. For whatever reason, flaps can return
"App not found" or "Could not find app" when it has an issue finding an
app. We're doing string comparisons :vomit: to find both
I realize that I'm not correctliy recording the status codes in the way
that I wanted. We're now always recording flaps error status codes
@billyb2 billyb2 marked this pull request as ready for review June 25, 2024 17:33
@rugwirobaker
Copy link
Contributor

Could the retry code be added to the client code itself? I think it would be more clean.

@billyb2
Copy link
Contributor Author

billyb2 commented Jun 26, 2024

Could the retry code be added to the client code itself? I think it would be more clean.

yea that's a good idea

})

if err != nil {
if strings.Contains(err.Error(), "machine still restarting") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Small neat but switch statements are more readable

@billyb2
Copy link
Contributor Author

billyb2 commented Jul 30, 2024

Closing, superseded by #3785

@billyb2 billyb2 closed this Jul 30, 2024
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