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

[LLC] [Core] Resolve design concerns about pipeline #24164

Closed
annelo-msft opened this issue Sep 22, 2021 · 4 comments
Closed

[LLC] [Core] Resolve design concerns about pipeline #24164

annelo-msft opened this issue Sep 22, 2021 · 4 comments
Assignees
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.

Comments

@annelo-msft
Copy link
Member

annelo-msft commented Sep 22, 2021

Continuing discussion started in this PR: #24122 (comment)

Was: Resolve design concerns about response error classification - context on this below

In addition, we would like to use this work on the pipeline design to resolve the issue that we are throwing when the HttpPipeline public constructor is called AND a policy is added to the call via RequestOptions.

@annelo-msft annelo-msft added Azure.Core Client This issue points to a problem in the data-plane of the library. labels Sep 22, 2021
@KrzysztofCwalina
Copy link
Member

Here is some context:

I have to say something still feels hacky about our error logic in the pipeline. It seems wrong that Response.IsError needs to be done with a custom policy. Error classification is such a fundamental thing that I think it should be done regardless of what policies are in the pipeline.

It seems wrong that the classifier is set on the pipeline. It should be a parameter to the pipeline.Send method. The method would set it on the message. Message would be a fundamental object that flows through the system and Response is just a "view" over a message, i.e. Response has a pointer to message not the other way.

If the pipeline was designed this way, the error feature would be trivial to implement: a) message has internal IsError property that simply calls the classifier stored in message. b) response, having a pointer to message, would just call _message.IsError.

This is just an example of a feature that would get easier with this architecture. I feel like this would help with many other problems we ran into in the past.

Can we still refactor the system to be like that?

@annelo-msft
Copy link
Member Author

More relevant context about alternative designs:

We chose the pipeline policy approach because there is a concern about IsError not being set correctly as the Response flows through the pipeline:
#24122 (comment)

An alternate approach (setting IsError in SendAsync) is illustrated here: 243a191#diff-0a59c43e2383e8557538b07995896820037da860dd0a133824970499a259b58eR73

We needed to modify the definition of SendAsync, and we had some concerns regarding potential perf impact here:
a780216

Next steps here include having a conversation around the tradeoffs of setting IsError at the beginning of the Response's lifetime vs. right before it exits the pipeline.

@annelo-msft annelo-msft changed the title [LLC] [Core] Resolve design concerns about response error classification [LLC] [Core] Resolve design concerns about pipeline Nov 10, 2021
@annelo-msft
Copy link
Member Author

I have to say something still feels hacky about our error logic in the pipeline. It seems wrong that Response.IsError needs to be done with a custom policy. Error classification is such a fundamental thing that I think it should be done regardless of what policies are in the pipeline.

This now happens in the transport policy.

@annelo-msft
Copy link
Member Author

Initial issue is addressed, and creating #25565 to track remaining.

@annelo-msft annelo-msft self-assigned this Nov 30, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2023
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.
Projects
None yet
Development

No branches or pull requests

2 participants