Actor State TTL #1164

JoshVanL · 2023-10-12T14:30:05Z

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 #1164

Signed-off-by: joshvanl <me@joshvanl.dev>

JoshVanL · 2023-10-12T15:12:45Z

src/Dapr.Actors/DaprHttpInteractor.cs

@@ -57,7 +57,7 @@ internal class DaprHttpInteractor : IDaprInteractor
            this.httpClient.Timeout = requestTimeout ?? this.httpClient.Timeout;
        }

-        public async Task<string> GetStateAsync(string actorType, string actorId, string keyName, CancellationToken cancellationToken = default)
+        public async Task<ActorStateResponse<string>> GetStateAsync(string actorType, string actorId, string keyName, CancellationToken cancellationToken = default)


We want to keep ActorStateResponse as an internal type. How do we keep complication happy with this constraint?

@philliphoff any ideas here?

ActorStateResponse is already internal, no, or are you referring to the fact that this method is public? IDaprHttpInteractor and DaprHttpInteractor are already internal types, so nothing of them can be seen outside the SDK. The public on the method only applies to other types within the SDK. (Method visibility is always masked by visibility of the underlying type.)

philliphoff · 2023-10-13T06:23:19Z

src/Dapr.Actors/Runtime/ActorStateManager.cs

@@ -35,17 +36,17 @@ internal ActorStateManager(Actor actor)
            this.defaultTracker = new Dictionary<string, StateMetadata>();
        }

-        public async Task AddStateAsync<T>(string stateName, T value, CancellationToken cancellationToken)
+        public async Task AddStateAsync<T>(string stateName, T value, CancellationToken cancellationToken, int? ttlInSeconds)


Would it make sense to instead add an overload for when the TTL is specified:
AddStateAsync<T>(string stateName, T value, int ttlInSeconds) rather than deviate from the .NET guidance that CancellationToken be the last parameter of a method declaration. An exception exists to maintain backward compatibility with existing public APIs, but an overload would seem to provide the same effect while maintaining a consistent API.

Also, is it important for TTL to be specified in integer seconds as opposed to, say, a TimeSpan, which might be more usable for larger values? (What do we expect the typical TTL to be?)

Thanks @philliphoff! I'm brand new to .Net so please forgive my noob mistakes 😄 Can you take a look at my new changes to see if that is what you are after?

Also, is it important for TTL to be specified in integer seconds as opposed to, say, a TimeSpan, which might be more usable for larger values? (What do we expect the typical TTL to be?)

Indeed, TTL should be given as seconds as this matches what the Dapr API is expecting. What TTL to use is highly dependent on how long the app will be using the state for, but generally it should be set to "a bit more" than the expected usefulness of the key.

Right. I was think more of the actual values used for TTLs. For example, the samples specify a TTL of 360. Would that be better expressed as TimeSpan.FromMinutes(3) so that users need not do the math (and potentially get it wrong), specially if they have much larger TTLs (e.g. hours/days).

Gotcha, yes that makes sense I'll add that in.

src/Dapr.Actors/Communication/ActorStateResponse.cs

Signed-off-by: joshvanl <me@joshvanl.dev>

philliphoff · 2023-10-16T23:06:47Z

src/Dapr.Actors/Runtime/ActorStateChange.cs

+        /// <remarks>
+        /// If null, the state will not expire.
+        /// </remarks>
+        public int? TTLInSeconds {


Since this conversion to seconds is only used in a single place (for the specific circumstances of the runtime API), would it make sense to move this logic there and only expose TTLExpireTime? Is there value in exposing both to users of ActorStateChange? (Especially since this property is a constantly changing value, being relative to the current date/time, which users may not expect from a property.)

Thanks @philliphoff, I've updated the PR to only have hold the TTLExpireTime value which gets converted to int seconds when saving the actor state.

change metadata Signed-off-by: joshvanl <me@joshvanl.dev>

philliphoff · 2023-10-24T18:36:24Z

src/Dapr.Actors/Runtime/ActorStateManager.cs

-                // Check if the property was marked as remove in the cache
-                if (stateMetadata.ChangeKind == StateChangeKind.Remove)
+                // Check if the property was marked as remove in the cache or is expired
+                if (stateMetadata.ChangeKind == StateChangeKind.Remove || stateMetadata.TTLExpireTime <= DateTime.UtcNow)


Nit: DateTimeOffset.UtcNow (both here and elsewhere in the changes)?

I'm virtually certain that the built-in coersion between DateTime and DateTimeOffset make what's there equivalent, but perhaps best to use DateTimeOffset as consistently as possible.

Also might be useful to keep a watch on .NET 8 which (finally) adds a means of abstracting time to simplify testing.

Thanks! Done

philliphoff · 2023-10-24T18:44:48Z

src/Dapr.Actors/Runtime/ActorStateManager.cs

-            public static StateMetadata Create<T>(T value, StateChangeKind changeKind)
+            public DateTimeOffset? TTLExpireTime { get; set; }
+
+            public static StateMetadata Create<T>(T value, StateChangeKind changeKind, DateTimeOffset? ttlExpireTime = null, TimeSpan? ttl = null)


Is the intent to have a method which allows a TTL, expiry time, or neither? If so, consider three methods, one which doesn't accept either, one that accepts the offset, and a third that accepts the time span. That would help ensure proper usage.

Yes, that makes sense- they are intended to be mutually exclusive. I've moved to using 3 functions.

src/Dapr.Actors/Runtime/ActorStateManager.cs

philliphoff · 2023-10-24T18:52:11Z

src/Dapr.Actors/Runtime/DaprStateProvider.cs

@@ -132,6 +133,12 @@ private async Task DoStateChangesTransactionallyAsync(string actorType, string a
                            writer.WritePropertyName("value");
                            JsonSerializer.Serialize(writer, stateChange.Value, stateChange.Type, jsonSerializerOptions);
                        }
+
+                        if (stateChange.TTLExpireTime.HasValue) {
+                            var ttl = (int)Math.Ceiling((stateChange.TTLExpireTime.Value - DateTime.UtcNow).TotalSeconds);


Just a question, but is it spec'd to use the ceiling (vs. the floor or rounding) for TTLs?

The spec is that it needs to be an integer value. It makes sense to me to be consistent in how we translate a fractional seconds. Rounding up a fractional second ensures that a value has a TTL of 1 second, rather than 0.

Signed-off-by: joshvanl <me@joshvanl.dev>

codecov · 2023-10-31T14:04:40Z

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 ⚠️
src/Dapr.Actors/Runtime/DaprStateProvider.cs	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            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	Coverage Δ
net6	`68.46% <54.05%> (+6.89%)`	⬆️
net7	`68.46% <54.05%> (+1.88%)`	⬆️
net8	`68.46% <54.05%> (+1.88%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JoshVanL · 2023-10-31T17:38:25Z

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

philliphoff · 2023-11-01T15:48:53Z

@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 · 2023-11-06T16:17:43Z

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

Signed-off-by: joshvanl <me@joshvanl.dev>

src/Dapr.Actors/DaprHttpInteractor.cs

src/Dapr.Actors/Runtime/ActorStateManager.cs

src/Dapr.Actors/Runtime/DaprStateProvider.cs

philliphoff · 2023-11-16T19:54:58Z

@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.).

philliphoff

Needs some changes (added as suggestion comments) in order to resolve E2E failure.

Signed-off-by: joshvanl <me@joshvanl.dev>

JoshVanL · 2023-11-17T20:34:05Z

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 🙂

Signed-off-by: joshvanl <me@joshvanl.dev>

JoshVanL · 2023-11-27T10:40:08Z

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

JoshVanL added 3 commits October 12, 2023 14:56

Actor state TTL support

de0a847

Signed-off-by: joshvanl <me@joshvanl.dev>

Updates Dapr version to 1.12.0

2f0cd2a

Signed-off-by: joshvanl <me@joshvanl.dev>

Adds ttlInSeconds to example Dapr application

0718834

Signed-off-by: joshvanl <me@joshvanl.dev>

JoshVanL commented Oct 12, 2023

View reviewed changes

philliphoff reviewed Oct 13, 2023

View reviewed changes

src/Dapr.Actors/Communication/ActorStateResponse.cs Outdated Show resolved Hide resolved

JoshVanL added 2 commits October 13, 2023 09:27

Use overloads when setting ttlInSeconds. Don't use nullable int

9e4ada7

Signed-off-by: joshvanl <me@joshvanl.dev>

Use DateTimeOffset? rather than DateTime?

f3866e7

Signed-off-by: joshvanl <me@joshvanl.dev>

philliphoff reviewed Oct 16, 2023

View reviewed changes

Use TimeSpan instead of int seconds, and only expose TTLExpireTime in

ea9999b

change metadata Signed-off-by: joshvanl <me@joshvanl.dev>

JoshVanL marked this pull request as ready for review October 19, 2023 13:10

JoshVanL requested review from a team as code owners October 19, 2023 13:10

philliphoff reviewed Oct 24, 2023

View reviewed changes

src/Dapr.Actors/Runtime/ActorStateManager.cs Outdated Show resolved Hide resolved

philliphoff reviewed Oct 24, 2023

View reviewed changes

JoshVanL added 2 commits October 27, 2023 18:13

Address feedback

df5a6c0

Signed-off-by: joshvanl <me@joshvanl.dev>

Fix TestDaprInteractor

5922940

Signed-off-by: joshvanl <me@joshvanl.dev>

JoshVanL added 7 commits November 13, 2023 13:56

Revert github actions Dapr CI changes

982d100

Signed-off-by: joshvanl <me@joshvanl.dev>

Register StateActor in E2E

5bb19da

Signed-off-by: joshvanl <me@joshvanl.dev>

Add State namespace

c4135c7

Signed-off-by: joshvanl <me@joshvanl.dev>

Merge branch 'master' into actor-state-ttl

f57b549

Fix Actor state test TTL exception check

cd58e97

Signed-off-by: joshvanl <me@joshvanl.dev>

Merge branch 'master' into actor-state-ttl

29265a0

Merge branch 'master' into actor-state-ttl

9c381d6

philliphoff reviewed Nov 16, 2023

View reviewed changes

src/Dapr.Actors/DaprHttpInteractor.cs Outdated Show resolved Hide resolved

philliphoff reviewed Nov 16, 2023

View reviewed changes

src/Dapr.Actors/Runtime/ActorStateManager.cs Outdated Show resolved Hide resolved

philliphoff reviewed Nov 16, 2023

View reviewed changes

src/Dapr.Actors/Runtime/ActorStateManager.cs Show resolved Hide resolved

philliphoff reviewed Nov 16, 2023

View reviewed changes

src/Dapr.Actors/Runtime/DaprStateProvider.cs Outdated Show resolved Hide resolved

philliphoff requested changes Nov 16, 2023

View reviewed changes

JoshVanL added 2 commits November 17, 2023 20:24

Fix-up actor ttl state bugs and add unit tests

6d5728d

Signed-off-by: joshvanl <me@joshvanl.dev>

Return false in state provider if key is expired

ba7ab5e

Signed-off-by: joshvanl <me@joshvanl.dev>

Ensure enough time has passed before checking key

2967d4a

Signed-off-by: joshvanl <me@joshvanl.dev>

JoshVanL mentioned this pull request Nov 27, 2023

[Proposal]: Actor State TTL dapr/dapr#5899

Open

5 tasks

Merge branch 'master' into actor-state-ttl

d9d66b6

philliphoff approved these changes Dec 5, 2023

View reviewed changes

halspang approved these changes Jan 8, 2024

View reviewed changes

Merge branch 'master' into actor-state-ttl

a1b0b00

halspang merged commit 233c620 into dapr:master Jan 8, 2024
11 of 12 checks passed

halspang modified the milestone: v1.13 Jan 8, 2024

halspang mentioned this pull request Jan 8, 2024

FEATURE PREVIEW: Add Actor State TTL support #1078

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Actor State TTL #1164

Actor State TTL #1164

JoshVanL commented Oct 12, 2023 •

edited

Loading

JoshVanL Oct 12, 2023

JoshVanL Oct 24, 2023

philliphoff Oct 24, 2023 •

edited

Loading

philliphoff Oct 13, 2023

JoshVanL Oct 13, 2023

philliphoff Oct 16, 2023

JoshVanL Oct 17, 2023

philliphoff Oct 16, 2023

JoshVanL Oct 17, 2023

philliphoff Oct 24, 2023 •

edited

Loading

philliphoff Oct 24, 2023

JoshVanL Oct 27, 2023

philliphoff Oct 24, 2023

JoshVanL Oct 27, 2023

philliphoff Oct 24, 2023

JoshVanL Oct 27, 2023

codecov bot commented Oct 31, 2023 •

edited

Loading

JoshVanL commented Oct 31, 2023

philliphoff commented Nov 1, 2023

JoshVanL commented Nov 6, 2023

philliphoff commented Nov 16, 2023

philliphoff left a comment

JoshVanL commented Nov 17, 2023

JoshVanL commented Nov 27, 2023

Actor State TTL #1164

Actor State TTL #1164

Conversation

JoshVanL commented Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philliphoff Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philliphoff Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 31, 2023 • edited Loading

Codecov Report

JoshVanL commented Oct 31, 2023

philliphoff commented Nov 1, 2023

JoshVanL commented Nov 6, 2023

philliphoff commented Nov 16, 2023

philliphoff left a comment

Choose a reason for hiding this comment

JoshVanL commented Nov 17, 2023

JoshVanL commented Nov 27, 2023

JoshVanL commented Oct 12, 2023 •

edited

Loading

philliphoff Oct 24, 2023 •

edited

Loading

philliphoff Oct 24, 2023 •

edited

Loading

codecov bot commented Oct 31, 2023 •

edited

Loading