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

ClientModel: Address extensibility for IsRetriable to validate that is additive after v1. #41465

Closed
annelo-msft opened this issue Jan 22, 2024 · 4 comments
Assignees
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. CoreDi 🚀 🔮 System.ClientModel Base Core library
Milestone

Comments

@annelo-msft
Copy link
Member

Understand how we would add the ability for end-users to specify what Azure.Core supports in the ResponseClassifier.IsRetriable API today, to ensure we have the correct design for PipelineMessageClassifier.

@tg-msft FYI

@annelo-msft annelo-msft added Client This issue points to a problem in the data-plane of the library. Azure.Core CoreDi 🚀 🔮 System.ClientModel Base Core library labels Jan 22, 2024
@annelo-msft annelo-msft added this to the 2024-02 milestone Jan 22, 2024
@annelo-msft annelo-msft self-assigned this Jan 22, 2024
@annelo-msft
Copy link
Member Author

Related to #41320

@annelo-msft
Copy link
Member Author

annelo-msft commented Jan 22, 2024

The problem we have today in Azure.Core is that if a client-author provides a client-scoped ResponseClassifier and the generated protocol method provides a method-scoped ResponseClassifier, since classifiers are not composable, we don't know how to choose which one to apply when we call classifier.IsRetriable(message) in the retry policy.

We addressed this problem for ResponseClassifier error classification by adding AddClassifier methods to RequestContext that allow either adding a status code classification into the generated method-scoped classifier, or adding a ResponseClassificationHandler to a chain of classifiers, where each handler designates both whether it wants to classify the message and then, if it does, the message "is error response" classification. The order of the handlers in the chain allows us to let end-user classification handlers take precedence over generated classifier logic, while preserving the generated classifier logic where the handler doesn't have an opinion.

The above solves the composability problem for error classification, but does not solve it for composition of retriability. And, in fact, I think it points to a challenge where we've named the error-classification APIs on RequestContext in such a way that they may imply classifiers are only applicable to error-response classification and response-retry classification. Maybe we can find a way out of that.

In System.ClientModel, though, I think we can solve this similar to how we've said we'll address the addition of error-classifiers in #41320. We can choose to add IsRetriable to PipelineMessageClassifier -- we know from the Azure.Core ResponseClassifer APIs that the name of IsErrorResponse will allow the addition of multiple IsRetriable* methods, where the base class provides a default implementation for each. What we might want to do differently in ClientModel's RequestOptions when we add composability of both error classifiers and retry classifiers is name rename the AddClassifier methods to AddErrorClassifier and AddRetryClassifier. We retain the public MessageClassifier property on PipelineMessage, and it remains the client-author's responsibility to compose classifiers before setting this value on the message -- for both error classification AND retry classification -- and we should be careful that we don't allow a MessageClassifier to be specified in PipelineOptions per this comment.

@annelo-msft
Copy link
Member Author

Ah, it looks like we may still need validation that the ChainingClassifier implementation would work if MessageClassifier had both error and retry classification logic.

@annelo-msft
Copy link
Member Author

Closed by #41586

@github-actions github-actions bot locked and limited conversation to collaborators Apr 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. CoreDi 🚀 🔮 System.ClientModel Base Core library
Projects
None yet
Development

No branches or pull requests

1 participant