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

Perf: unset EnableContentResponseOnWrite dotnet#22999 #23322

Merged

Conversation

umitkavala
Copy link
Contributor

I've create draft version to check implementation is correct.
See https://devblogs.microsoft.com/cosmosdb/enable-content-response-on-write/

@dnfadmin
Copy link

dnfadmin commented Nov 13, 2020

CLA assistant check
All CLA requirements met.

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Nov 13, 2020

Generally looking good, just add ETag tests as mentioned in #22999 (comment)

@AndriySvyryd AndriySvyryd self-assigned this Nov 13, 2020
@AndriySvyryd
Copy link
Member

AndriySvyryd commented Nov 13, 2020

Also if the option hasn't been set to any value you can check whether we'll need the content at all: jObjectProperty?.ValueGenerated == ValueGenerated.OnAddOrUpdate

@umitkavala
Copy link
Contributor Author

Thanks @AndriySvyryd I'll have do required changes.

@umitkavala
Copy link
Contributor Author

umitkavala commented Nov 13, 2020

I've a problem with access option value
Its not good idea to cast like this.
var enableContentResponseOnWrite = ((ICosmosSingletonOptions)Client.ClientOptions).EnableContentResponseOnWrite;
And realized that CosmosClientOptions does not have property for that. So using option is not possible I guess.
Do you have any suggestion?

@AndriySvyryd
Copy link
Member

You can now convert this to a non-draft PR to allow the tests to run

@umitkavala umitkavala marked this pull request as ready for review November 15, 2020 14:16
moved ETag tests to the ConcurencyTest class
@umitkavala
Copy link
Contributor Author

umitkavala commented Nov 15, 2020

Seems we can only have 1 constructor on DBcontext. so I couldn't add new constructor but handled with CreateOptions using default constructor of ConcurrencyContext

@AndriySvyryd AndriySvyryd merged commit 89816b7 into dotnet:main Nov 16, 2020
@AndriySvyryd
Copy link
Member

Thanks for your contribution!

@umitkavala
Copy link
Contributor Author

wow. I'm so happy to contribute :) How can I track this change will release?
I'll continue to contribute of course!

@umitkavala umitkavala deleted the unset_enableContentResponseOnWrite branch November 16, 2020 08:34
@AndriySvyryd
Copy link
Member

It should already be available in daily builds of 6.0.0
We'll start releasing previews next year.

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.

4 participants