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.

Sign up for GitHub

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

Jump to bottom

DefaultTimeToLiveInMs specified in milliseconds? #51

Closed
shibayan opened this issue Aug 23, 2021 · 5 comments · Fixed by #52
Closed

DefaultTimeToLiveInMs specified in milliseconds? #51

shibayan opened this issue Aug 23, 2021 · 5 comments · Fixed by #52
Assignees
@ealsur
Labels
bug Something isn't working

Comments

@shibayan
Copy link

Describe the bug

I thought the value to be specified for DefaultTimeToLiveInMs was in milliseconds from the property name, but it seems that it actually needs to be specified in seconds.

It would be easier to understand if the units could be clarified in the document comments.
@ealsur ealsur added bug Something isn't working and removed needs-investigation labels Aug 23, 2021
@ealsur
Copy link
Member

ealsur commented Aug 23, 2021

Microsoft.Extensions.Caching.Cosmos/src/CosmosCache.cs

Lines 402 to 403 in d0a1a60

int defaultTimeToLive = this.options.DefaultTimeToLiveInMs.HasValue
&& this.options.DefaultTimeToLiveInMs.Value > 0 ? this.options.DefaultTimeToLiveInMs.Value : CosmosCache.DefaultTimeToLive;

This is a bug, the value is not being correctly changed when it's passed down. Sadly at this point we cannot change the property name because it would be considered a breaking change.

The service property is actually in seconds (https://docs.microsoft.com/en-us/azure/cosmos-db/time-to-live#time-to-live-for-containers-and-items), it was a miss that this property's name actually contains the suffix InMs.

I'll send a PR to fix it.

@ealsur ealsur mentioned this issue Aug 23, 2021
Merged
@ealsur
Copy link
Member

ealsur commented Aug 23, 2021

@shibayan please confirm if you need a release to cover this gap.

@shibayan
Copy link
Author

@ealsur Thanks for the quick response. This change looks good to me.
I hope the property name will be changed in the next major version update.

@ealsur ealsur mentioned this issue Aug 23, 2021
Open
@ealsur
Copy link
Member

ealsur commented Aug 23, 2021

@shibayan Created a tracking issue.

@ealsur
Copy link
Member

ealsur commented Aug 24, 2021

New version with the fix released https://www.nuget.org/packages/Microsoft.Extensions.Caching.Cosmos/1.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ealsur ealsur

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix DefaultTimeToLiveInMs handling when creating container Azure/Microsoft.Extensions.Caching.Cosmos
2 participants
@shibayan @ealsur