dapr / dotnet-sdk

Dapr SDK for .NET
Apache License 2.0
1.12k stars 340 forks source link

Actor State TTL #1164

Closed JoshVanL closed 11 months ago

JoshVanL commented 1 year ago

Dapr Actor state TTL is a feature that was added in Dapr 1.12.

Adds support for Actor State TTL. TTL of actor state is set by using the ttlInSeconds metadata option when saving actor state. The actor state cache uses the ttlExpireTime return metadata field to determine whether the key is still valid, which is important as the cache may become populated with TTL keys which were not created by the current manager.

SDK developers are encouraged to use TTLs when storing any actor state in order to prevent stale data accumulating in the actor state store over time.

Closes https://github.com/dapr/dotnet-sdk/pull/1164

codecov[bot] commented 1 year ago

Codecov Report

Attention: 51 lines in your changes are missing coverage. Please review.

Comparison is base (7616bfa) 66.58% compared to head (a1b0b00) 68.47%.

Files Patch % Lines
src/Dapr.Actors/Runtime/ActorStateManager.cs 39.02% 44 Missing and 6 partials :warning:
src/Dapr.Actors/Runtime/DaprStateProvider.cs 92.85% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1164 +/- ## ========================================== + Coverage 66.58% 68.47% +1.88% ========================================== Files 171 172 +1 Lines 5752 5846 +94 Branches 628 648 +20 ========================================== + Hits 3830 4003 +173 + Misses 1772 1681 -91 - Partials 150 162 +12 ``` | [Flag](https://app.codecov.io/gh/dapr/dotnet-sdk/pull/1164/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dapr) | Coverage Δ | | |---|---|---| | [net6](https://app.codecov.io/gh/dapr/dotnet-sdk/pull/1164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dapr) | `68.46% <54.05%> (+6.89%)` | :arrow_up: | | [net7](https://app.codecov.io/gh/dapr/dotnet-sdk/pull/1164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dapr) | `68.46% <54.05%> (+1.88%)` | :arrow_up: | | [net8](https://app.codecov.io/gh/dapr/dotnet-sdk/pull/1164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dapr) | `68.46% <54.05%> (+1.88%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dapr#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JoshVanL commented 1 year ago

@philliphoff do you understand why the integration tests are failing? I'm not too sure what the problem is.

philliphoff commented 1 year ago

@philliphoff do you understand why the integration tests are failing? I'm not too sure what the problem is.

Dunno. Maybe retry the job in case it was just a transient issue?

JoshVanL commented 1 year ago

@philliphoff Doesn't seem to be the case. Struggling to understand what is going wrong here.. at the limit of my .NET knowledge :grimacing:

philliphoff commented 1 year ago

@philliphoff Doesn't seem to be the case. Struggling to understand what is going wrong here.. at the limit of my .NET knowledge 😬

Poked around this morning and found a couple of issues where existing state was ignored in favor of default values (see above suggestions), which were causing the E2E to fail. It might be good to add some unit tests for DaprHttpInteractor, ActorStateManager, and DaprStateProvider, if reasonably possible, to ensure that all the cases are covered (e.g. when values are cached vs. non-cached), pre- and post-expiration, etc.).

JoshVanL commented 1 year ago

Thank you @philliphoff, really appreciate your help with this!

I've gone ahead and added those fixes, expanded the E2E state tests and added unit tests for those classed. Please take another look :slightly_smiling_face:

JoshVanL commented 1 year ago

Hi @philliphoff, please let me know what you think when you have time