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

creates a sqs queue for retry failed scale-up requests #1947

Merged
merged 4 commits into from
Jan 31, 2023

Conversation

jeanschmidt
Copy link
Contributor

No description provided.

@vercel
Copy link

vercel bot commented Jan 30, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
torchci ⬜️ Ignored (Inspect) Jan 31, 2023 at 11:52AM (UTC)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 30, 2023
for (const evt of evtFailed) {
const body: ActionRequestMessage = JSON.parse(evt.body);
const retryCount = body?.retryCount ?? 0;
const delaySeconds = Math.max(body?.delaySeconds ?? Config.Instance.retryScaleUpRecordDelayS / 2, 10);
Copy link
Contributor

@izaitsevfb izaitsevfb Jan 30, 2023

Choose a reason for hiding this comment

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

Nit.
The delay calculation formula looks a bit confusing. I'd rewrite it in a more explicit manner, like:

          if (body.delaySeconds === null) {
            body.delaySeconds = Config.Instance.retryScaleUpRecordDelayS;
          } else {
            body.delaySeconds *= 2;
          }
          body.delaySeconds = Math.max(body.delaySeconds, 10);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, much more readable this way

Copy link
Contributor

@izaitsevfb izaitsevfb left a comment

Choose a reason for hiding this comment

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

Overall, looks good. I'd appreciate the answer to this question, but, in any case, the change should work.

@jeanschmidt jeanschmidt merged commit 9afcf35 into main Jan 31, 2023
@jeanschmidt jeanschmidt deleted the jeanschmidt/retry_request_sqs branch January 31, 2023 13:49
seemethere added a commit that referenced this pull request Feb 7, 2023
…re retryable (such as lack of capacity) (#1947)"

This reverts commit 9afcf35.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants