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

fix: Throw HTTP Status 429 error when there are too many get Sagemaker Presigned URL requests #942

Merged
merged 6 commits into from
Mar 9, 2022

Conversation

nguyen102
Copy link
Contributor

@nguyen102 nguyen102 commented Mar 7, 2022

Issue #, if available:

Description of changes:
When multiple requests are made to retrieve a Sagemaker URL, we can not process more than one request at a time.

We'll now throw a HTTP Status Code 429 error, with the following body

429 error
{
    "requestId": "",
    "code": "tooManyRequests",
    "message": "Please wait 30 seconds before requesting Sagemaker URL"
}

Before the fix this was what we returned

500 error
{
    "requestId": "",
    "code": "internalError",
    "message": "Could not obtain a lock"
}

Checklist:

  • Have you successfully deployed to an AWS account with your changes?
  • Have you written new tests for your core changes, as applicable?

AS review ticket id:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Mar 7, 2022

Codecov Report

Merging #942 (baee991) into develop (15eb4d3) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head baee991 differs from pull request most recent head 06d8b0f. Consider uploading reports for the commit 06d8b0f to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #942      +/-   ##
===========================================
+ Coverage    51.45%   51.47%   +0.01%     
===========================================
  Files          295      295              
  Lines        16790    16795       +5     
  Branches      2598     2600       +2     
===========================================
+ Hits          8640     8645       +5     
  Misses        7163     7163              
  Partials       987      987              
Impacted Files Coverage Δ
...rvice-catalog/environment-sc-connection-service.js 61.49% <0.00%> (+1.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15eb4d3...06d8b0f. Read the comment docs.

@nguyen102 nguyen102 marked this pull request as ready for review March 8, 2022 21:49
@nguyen102 nguyen102 requested a review from a team as a code owner March 8, 2022 21:49
Tim Nguyen added 3 commits March 9, 2022 13:18
}
});
throw e;
Copy link
Contributor

@SanketD92 SanketD92 Mar 9, 2022

Choose a reason for hiding this comment

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

Are we throwing the stack trace itself? Rather could we throw a boom error with just the e.code if required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line will rethrow what was already thrown. In the code, it looks like that will be boom errors. That means there won't be a stack trace.

@nguyen102 nguyen102 merged commit 3dea763 into awslabs:develop Mar 9, 2022
@nguyen102 nguyen102 deleted the sagemaker-too-many-requests branch June 5, 2023 19:13
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