Add Actor Reminder/Timer TTL support by halspang · Pull Request #912 · dapr/dotnet-sdk

halspang · 2022-07-22T20:20:45Z

Description

This commit adds the TTL field to Actor reminders/timers. This allows
reminders and timers to expire after a given TimeSpan instead of
having to be manually deleted.

#788

Signed-off-by: Hal Spang halspang@microsoft.com

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #788

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Extended the documentation

codecov · 2022-07-22T20:29:00Z

Codecov Report

Merging #912 (d235eb7) into master (e58b1de) will decrease coverage by 0.31%.
The diff coverage is 40.36%.

@@            Coverage Diff             @@
##           master     #912      +/-   ##
==========================================
- Coverage   70.11%   69.79%   -0.32%     
==========================================
  Files         152      154       +2     
  Lines        4939     5079     +140     
  Branches      543      549       +6     
==========================================
+ Hits         3463     3545      +82     
- Misses       1349     1402      +53     
- Partials      127      132       +5

Flag	Coverage Δ
net5	`69.69% <40.36%> (-0.36%)`	⬇️
net6	`69.69% <40.36%> (-0.32%)`	⬇️
netcoreapp3.1	`69.75% <40.36%> (-0.32%)`	⬇️

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

Impacted Files	Coverage Δ
...rc/Dapr.Actors/Runtime/DefaultActorTimerManager.cs	`12.00% <0.00%> (ø)`
src/Dapr.Actors/Runtime/ActorReminder.cs	`19.60% <13.51%> (-23.25%)`	⬇️
src/Dapr.Actors/Runtime/ActorTimer.cs	`18.64% <14.63%> (-18.40%)`	⬇️
src/Dapr.Actors/Runtime/Actor.cs	`72.80% <50.98%> (-15.43%)`	⬇️
src/Dapr.Actors/Runtime/TimerInfo.cs	`72.00% <54.54%> (-3.76%)`	⬇️
src/Dapr.Actors/Runtime/ActorReminderOptions.cs	`100.00% <100.00%> (ø)`
src/Dapr.Actors/Runtime/ActorTimerOptions.cs	`100.00% <100.00%> (ø)`
src/Dapr.Actors/Runtime/ReminderInfo.cs	`97.43% <100.00%> (+97.43%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

This commit adds the TTL field to Actor reminders/timers. This allows reminders and timers to expire after a given TimeSpan instead of having to be manually deleted. dapr#788 Signed-off-by: Hal Spang <halspang@microsoft.com>

rynowak · 2022-07-29T17:01:41Z

-            await proxy.RegisterTimer();
-            await proxy.RegisterReminder();
+            await proxy.RegisterTimer(null);
+            await proxy.RegisterReminder(null);


nit: if this is a default-valued parameter you don't have to pass null here.

The ActorProxy complained about the interface having a default value and without that, the top level doesn't appear to accept the default parameter. I'll look again as maybe I was just being wrong, but that's the behavior I observed.

rynowak · 2022-07-29T17:04:07Z

            TimeSpan dueTime,
-            TimeSpan period)
+            TimeSpan period,
+            TimeSpan? ttl = null)


This is a breaking change. See: https://github.com/ltrzesniewski/dotnet-runtime/blob/master/docs/coding-guidelines/breaking-change-rules.md#signatures

This needs to be a new overload.

When you add the new overload, we probably can't make ttl optional, because that will cause an ambiguity in overload resolution.

Dang, I thought the default saved me from the breaking change. Thanks for letting me know! I'll fix this and apply throughout.

When you add the new overload, we probably can't make ttl optional, because that will cause an ambiguity in overload resolution.

So this was a problem because null is semantically important here unfortunately. I didn't want to put in something like looking for a specific TimeSpan value made us remove the TTL because who knows what people want to put in (and if it's wrong I'd rather error). So I introduced a class that allows us to pass in a null while keeping a consistent API surface. Let me know what you think, it's generally internal because it felt weird to expose it.

rynowak · 2022-07-29T17:20:43Z

            TimeSpan dueTime,
-            TimeSpan period)
+            TimeSpan period,
+            TimeSpan? ttl = null)


Same deal, this is a breaking change.

rynowak · 2022-07-29T17:21:59Z

            TimeSpan dueTime,
-            TimeSpan period)
+            TimeSpan period,
+            TimeSpan? ttl = null)


This is also a breaking change.

These are the best references I know of if you want to learn more:

https://github.com/ltrzesniewski/dotnet-runtime/blob/master/docs/coding-guidelines/breaking-changes.md

https://github.com/ltrzesniewski/dotnet-runtime/blob/master/docs/coding-guidelines/breaking-change-rules.md

https://github.com/ltrzesniewski/dotnet-runtime/blob/master/docs/coding-guidelines/breaking-change-definitions.md

I accidentally linked to someone's fork, but those should all be valid.

rynowak · 2022-07-29T17:24:59Z

            TimeSpan dueTime,
-            TimeSpan period)
+            TimeSpan period,
+            TimeSpan? ttl = null)


Also a breaking change :)

rynowak · 2022-07-29T17:25:19Z

            this.Data = data;
            this.DueTime = dueTime;
            this.Period = period;
+            this.Ttl = ttl;


This is not a breaking change 😁 - internal type

rynowak · 2022-07-29T17:26:00Z

            TimeSpan dueTime,
-            TimeSpan period)
+            TimeSpan period,
+            TimeSpan? ttl = null)


This is not a breaking change 👍 - internal type

Signed-off-by: Hal Spang <halspang@microsoft.com>

halspang requested review from a team as code owners July 22, 2022 20:20

halspang force-pushed the reminder_ttl branch 2 times, most recently from 92ae1a8 to d800f37 Compare July 22, 2022 20:26

halspang force-pushed the reminder_ttl branch 2 times, most recently from 0c6a8ca to 5e4198e Compare July 25, 2022 13:41

Add Actor Reminder/Timer TTL support

7d82b96

This commit adds the TTL field to Actor reminders/timers. This allows reminders and timers to expire after a given TimeSpan instead of having to be manually deleted. dapr#788 Signed-off-by: Hal Spang <halspang@microsoft.com>

halspang force-pushed the reminder_ttl branch from 5e4198e to 7d82b96 Compare July 25, 2022 14:25

rynowak reviewed Jul 29, 2022

View reviewed changes

Fix breaking changes with reminders/timers.

d235eb7

Signed-off-by: Hal Spang <halspang@microsoft.com>

rynowak approved these changes Aug 3, 2022

View reviewed changes

halspang merged commit 8bc7e90 into dapr:master Aug 3, 2022

Conversation

halspang commented Jul 22, 2022

Description

Issue reference

Checklist

Uh oh!

codecov Bot commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rynowak Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jul 22, 2022 •

edited

Loading

rynowak Jul 29, 2022 •

edited

Loading