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

Investigate unification of Azure.Core Response<T> and Cosmos Response<T> #7097

Closed
KrzysztofCwalina opened this issue Aug 2, 2019 · 4 comments
Assignees
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.

Comments

@KrzysztofCwalina
Copy link
Member

KrzysztofCwalina commented Aug 2, 2019

Cosmos sources: https://github.com/Azure/azure-cosmos-dotnet-v3/blob/master/Microsoft.Azure.Cosmos/src/Resource/Response.cs

We should not ship Response<T> in Cosmos. We should either unify to one Response<T>, or ship two "response" types in Azure.Core.

The reasons Cosmos has a separate response type are:

  1. It supports faster (supposedly) headers lookup (avoids dictionary access). We should measure performance and do optimizations in Response
  2. Allows for inherited response types. This is for both perf and for usability (adds strongly types APIs to subtypes). We should explore using extension methods for this.
  3. Cosmos Response<T> is not Disposable

Also, we should unify headers collection (even if we cannot unify response).

Lastly, this is related to the "Streaming" APIs in Cosmos, i.e. CosmosClient has two methods for each operation. One deserializes the model, and one optimized for efficient streaming access. It would be great if the unified Response supported streaming scenarios.

@KrzysztofCwalina KrzysztofCwalina added Azure.Core Client This issue points to a problem in the data-plane of the library. labels Aug 2, 2019
@j82w
Copy link

j82w commented Aug 2, 2019

Another big difference is Cosmos.Response is not disposable and has no access to the stream. This prevents users from needing to wrap every response in a using statement.

@pakrym
Copy link
Contributor

pakrym commented Aug 2, 2019

Related #6841

@j82w
Copy link

j82w commented Aug 2, 2019

In Cosmos we found that there was 2 types of users. One that wants performance and only wants to deal with streams. The other wants a simple typed world and doesn't want to deal with memory leaks from missing using statements. We originally tried doing a merged response like Azure.Core.Response, and found that it was a bad experience for both users. The solution we arrived at was having 2 different responses. One for the typed responses Response and one for the stream responses ResponseMessage.

@pakrym
Copy link
Contributor

pakrym commented Aug 13, 2019

FYI, we've merged a change making the Response non-disposable (#7273)

@pakrym pakrym closed this as completed Sep 6, 2019
@pakrym pakrym changed the title Unify Azure.Core Response<T> and Cosmos Response<T> Investigate unification of Azure.Core Response<T> and Cosmos Response<T> Sep 6, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Mar 29, 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

3 participants