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

Fixes processing 429s from SPs #2165

Merged
merged 3 commits into from
Aug 19, 2021
Merged

Conversation

brendankowitz
Copy link
Member

Description

Stored procedures return a statuscode 400 with a substatus when they fail. When this was a 429, we were not processing this correctly.
The errorcode in the SP was also not correct.

Related issues

Addresses [issue #].

Testing

Describe how this change was tested.

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 50 characters
  • Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
  • Tag the PR with the type of update: Bug, Dependencies, Enhancement, or New-Feature
  • Tag the PR with Azure API for FHIR if this will release to the managed service
  • Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

@brendankowitz brendankowitz requested a review from a team as a code owner August 12, 2021 21:04
@@ -145,6 +145,6 @@ function acquireExportJobs(maximumNumberOfConcurrentJobsAllowedInString, jobHear
}

function throwTooManyRequestsError() {
throw new Error(ErrorCodes.RequestEntityTooLarge, `The request could not be completed.`);
throw new Error(429, `The request could not be completed.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

429

ErrorCodes.TooManyRequests sound more appropriate than a const

Copy link
Member Author

Choose a reason for hiding this comment

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

This const isn't in provided out of the box :( http://azure.github.io/azure-cosmosdb-js-server/DocDbWrapperScript.js.html

I could create our own, but it is still defined per proc

@@ -24,6 +24,7 @@ function hardDelete(resourceTypeName, resourceId, keepCurrentVersion) {

let deletedResourceIdList = new Array();

throwTooManyRequestsError();
Copy link
Contributor

Choose a reason for hiding this comment

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

throwTooManyRequestsError

I don't get this addition

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, that was my manual test :(

@brendankowitz brendankowitz force-pushed the personal/bkowitz/cosmos-sp-429s branch from 443ba96 to 60e6209 Compare August 18, 2021 16:20
@brendankowitz brendankowitz merged commit 7872ebb into main Aug 19, 2021
@brendankowitz brendankowitz deleted the personal/bkowitz/cosmos-sp-429s branch August 19, 2021 18:21
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.

None yet

3 participants