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] Implement IsEnvironmentReadyAsync #35453

Merged
merged 3 commits into from
Apr 11, 2023

Conversation

joseharriaga
Copy link
Member

@joseharriaga joseharriaga commented Apr 10, 2023

Resolves: #35452.

Implemented IsEnvironmentReadyAsync with a simple request to the dynamic resource to confirm that permissions have propagated successfully, because otherwise we're at risk of having a bunch of test failures like this one:

Azure.RequestFailedException : Access denied due to invalid subscription key or wrong API endpoint. Make sure to provide a valid key for an active subscription and use a correct regional API endpoint for your resource.
Status: 401 (PermissionDenied)
ErrorCode: 401

Additionally, while testing, I found another instance of the transient Internal Server Error. The difference is that the message is simply "Internal Server Error." while the previous known case said "Failed to process task after several retry":

Azure.RequestFailedException : Internal Server Error.
Status: 200 (OK)
ErrorCode: InternalServerError

Additional Information:
Target: #/tasks/items/1
error-0: InternalServerError: Internal Server Error.

I added this one to the ShouldRetry method of RetryOnInternalServerErrorAttribute.

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@joseharriaga
Copy link
Member Author

/azp run net - textanalytics - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@joseharriaga
Copy link
Member Author

/azp run net - textanalytics - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@joseharriaga
Copy link
Member Author

/azp run net - textanalytics - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@joseharriaga
Copy link
Member Author

/azp run net - textanalytics - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@joseharriaga joseharriaga marked this pull request as ready for review April 10, 2023 21:56
@joseharriaga joseharriaga changed the title [Text Analytics] Implement IsEnvironmentReadyAsync [Text Analytics] Implement IsEnvironmentReadyAsync Apr 10, 2023
Copy link
Member

@deyaaeldeen deyaaeldeen 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, however, the issue seems to be not TA specific. Should the pipeline add some latency after the resource creation step to mitigate it?

@heaths
Copy link
Member

heaths commented Apr 10, 2023

Looks good, however, the issue seems to be not TA specific. Should the pipeline add some latency after the resource creation step to mitigate it?

We don't want to add latency to all pipelines. Few, hopefully, need it. See my review for what I suggested to inject latency when needed.

@joseharriaga joseharriaga merged commit 08af269 into Azure:main Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Text Analytics] Fix tests failing with "401 (PermissionDenied)"
5 participants