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

[Text Analytics] Fix rate limit reached issue #19249

Merged
merged 9 commits into from
Dec 14, 2021

Conversation

deyaaeldeen
Copy link
Member

Fixes #18941

Sometimes, we get rate limit reached error responses from the service that exceed the default number of retrys in the throttling retry policy.

For context, this particular API supports running multiple Text Analytics actions from one request, so the service does not like getting many successive requests for it. I also confirmed with their engineering team that the test suite is a bit too big.

To fix this, the test suite could be trimmed down but I am afraid that will affect the current coverage. Instead, I updated the retry policy for the test client to retry more.

@azure-sdk
Copy link
Collaborator

API changes have been detected in @azure/core-rest-pipeline. You can review API changes here

API changes

- export declare function throttlingRetryPolicy(): PipelinePolicy;
+ export declare function throttlingRetryPolicy(maxRetryCount?: number): PipelinePolicy;

@azure-sdk
Copy link
Collaborator

API changes have been detected in @azure/core-rest-pipeline. You can review API changes here

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

The core changes look great!

sdk/core/core-rest-pipeline/CHANGELOG.md Outdated Show resolved Hide resolved
sdk/core/core-rest-pipeline/package.json Show resolved Hide resolved
deyaaeldeen and others added 2 commits December 13, 2021 14:21
Co-authored-by: Jeff Fisher <xirzec@xirzec.com>
@deyaaeldeen deyaaeldeen enabled auto-merge (squash) December 13, 2021 22:25
@maorleger
Copy link
Member

Haven't reviewed this but I know @sadasant is working in this area as well - I just wanted to make him aware so his changes do not regress anything added here (or vice-versa) ❤️

Copy link
Contributor

@jeremymeng jeremymeng left a comment

Choose a reason for hiding this comment

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

Looks good to me! The throttling test is failing due to timeout. Could be related to the DEFAULT_CLIENT_MAX_RETRY_COUNT increase.

@deyaaeldeen
Copy link
Member Author

@sadasant I updated an identity test so that it conforms to the updated number of retries. Please let me know if there is any issues there.

@deyaaeldeen deyaaeldeen merged commit f31aa42 into Azure:main Dec 14, 2021
@deyaaeldeen deyaaeldeen deleted the textanalytics/fix-rate-limit branch December 14, 2021 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants