Skip to content

Conversation

@robertbrignull
Copy link
Contributor

@robertbrignull robertbrignull commented May 1, 2020

This should make the upload retry a few times and with a small amount of delay between attempts. Hopefully this will help get over very minor issues, though if there's a larger outage of github lasting more than a few seconds then it won't be able to help.

Merge / deployment checklist

Copy link
Contributor

@anaarmas anaarmas left a comment

Choose a reason for hiding this comment

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

I see we're re-trying not only on 5xx responses but on all. Additionally, the behaviour we had of not setting the job as failed when we get a 500 is changed too. Can you provide more of an insight as to why you've decided to go this way?

Copy link
Contributor

Choose a reason for hiding this comment

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

will be retry -> will retry

@robertbrignull
Copy link
Contributor Author

I see we're re-trying not only on 5xx responses but on all. Additionally, the behaviour we had of not setting the job as failed when we get a 500 is changed too. Can you provide more of an insight as to why you've decided to go this way?

Yes, thanks for spotting that. I completely forgot to mention that I made that change as part of this. I can easily back that change out if you'd prefer not to make it at this time.

I believe that the will of @jhutchings1 and @joshhale was to now fail the action if there's a 500 when uploading.

@robertbrignull
Copy link
Contributor Author

In fact, I will back this change out and we can make it at another time.

@robertbrignull
Copy link
Contributor Author

Changed to only retry on 5xx errors, and I've changed the error message to include the status code.

I've run some tests again so I'm happy with the testing of this.

' seconds: (' + statusCode + ') ' + await res.readBody());
// Sleep for the backoff period
await new Promise(r => setTimeout(r, backoffPeriods[attempt] * 1000));
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this continue here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To get to the next iteration of the loop. Technically it's not necessary as this is the last statement of the loop in this branch, but it doesn't hurt as far as I can see.

@robertbrignull robertbrignull merged commit 5bceb2b into master May 1, 2020
@robertbrignull robertbrignull deleted the upload_retry branch May 11, 2020 15:08
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