Added TTL param to actor timer and reminder #212

amulyavarote · 2022-03-14T19:40:06Z

Signed-off-by: Amulya Varote amulyavarote@microsoft.com

Description

Added TTL param to actor timer and reminder.

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: #141

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-03-14T19:48:12Z

Codecov Report

Merging #212 (3f08897) into master (7d9298a) will increase coverage by 0.15%.
The diff coverage is 67.74%.

@@            Coverage Diff             @@
##           master     #212      +/-   ##
==========================================
+ Coverage   30.56%   30.71%   +0.15%     
==========================================
  Files          70       72       +2     
  Lines        5644     5671      +27     
  Branches      172      177       +5     
==========================================
+ Hits         1725     1742      +17     
- Misses       3885     3892       +7     
- Partials       34       37       +3

Impacted Files	Coverage Δ
src/actors/client/ActorClient/ActorClientGRPC.ts	`3.41% <0.00%> (-0.13%)`	⬇️
src/actors/client/ActorClient/ActorClientHTTP.ts	`40.00% <0.00%> (-4.45%)`	⬇️
src/actors/runtime/AbstractActor.ts	`74.28% <ø> (ø)`
src/actors/runtime/ActorReminderData.ts	`50.00% <50.00%> (-2.95%)`	⬇️
test/actor/DemoActorReminderTtlImpl.ts	`88.88% <88.88%> (ø)`
test/actor/DemoActorTimerTtlImpl.ts	`88.88% <88.88%> (ø)`
test/actor/DemoActorReminder2Impl.ts	`87.50% <100.00%> (ø)`
test/actor/DemoActorReminderImpl.ts	`88.88% <100.00%> (ø)`
test/actor/DemoActorTimerImpl.ts	`88.88% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d9298a...3f08897. Read the comment docs.

XavierGeerinck · 2022-03-15T08:52:14Z

Is there a file missing? I see a commit with a test file but I can't find it in the review?

shubham1172 · 2022-03-16T12:42:13Z

daprdocs/content/en/js-sdk-docs/js-actors/_index.md

    dueTime: Temporal.Duration.from({ seconds: 2 }),
-    period: Temporal.Duration.from({ seconds: 1 })
+    period: Temporal.Duration.from({ seconds: 1 }),
+    ttl?: Temporal.Duration.from({ seconds: 1 }),


Suggested change

ttl?: Temporal.Duration.from({ seconds: 1 }),

ttl: Temporal.Duration.from({ seconds: 1 }),

shubham1172 · 2022-03-16T12:42:21Z

daprdocs/content/en/js-sdk-docs/js-actors/_index.md

  await client.actor.reminderCreate(DemoActorImpl.name, `actor-id`, `timer-id`, {
    dueTime: Temporal.Duration.from({ seconds: 2 }),
    period: Temporal.Duration.from({ seconds: 1 }),
+    ttl?: Temporal.Duration.from({ seconds: 1 }),


Suggested change

ttl?: Temporal.Duration.from({ seconds: 1 }),

ttl: Temporal.Duration.from({ seconds: 1 }),

shubham1172 · 2022-03-16T12:51:18Z

src/actors/runtime/AbstractActor.ts

   * @param reminderName name of the reminder
   * @param state the state to be send along with the reminder trigger
   * @param dueTime due time for the first trigger
+   * @param ttl time to live


Suggested change

* @param ttl time to live

* @param ttl time duration after which the reminder will be expired and deleted

shubham1172 · 2022-03-16T12:52:14Z

src/actors/runtime/AbstractActor.ts

  }

-  async registerActorTimer(timerName: string, callback: string, dueTime: Temporal.Duration, period: Temporal.Duration, state?: any) {
+  async registerActorTimer(timerName: string, callback: string, dueTime: Temporal.Duration, ttl: Temporal.Duration, period: Temporal.Duration, state?: any) {


Should we add a documentation comment for this function too?

shubham1172 · 2022-03-16T12:54:03Z

src/actors/runtime/AbstractActor.ts

   * @return Async void response
   */
-  async registerActorReminder<_Type>(reminderName: string, dueTime: Temporal.Duration, period: Temporal.Duration, state?: any) {
+  async registerActorReminder<_Type>(reminderName: string, dueTime: Temporal.Duration, ttl: Temporal.Duration, period: Temporal.Duration, state?: any) {


ttl should be optional, right?

shubham1172 · 2022-03-16T12:54:25Z

src/actors/runtime/AbstractActor.ts

  }

-  async registerActorTimer(timerName: string, callback: string, dueTime: Temporal.Duration, period: Temporal.Duration, state?: any) {
+  async registerActorTimer(timerName: string, callback: string, dueTime: Temporal.Duration, ttl: Temporal.Duration, period: Temporal.Duration, state?: any) {


Same as above for ttl being optional.

shubham1172 · 2022-03-16T12:55:12Z

src/actors/runtime/ActorReminderData.ts

   * @param period the time interval between reminder invocations after the first invocation
   */
-  constructor(reminderName: string, dueTime: number, period: number, state?: string | object) {
+  constructor(reminderName: string, dueTime: number, ttl: number, period: number, state?: string | object) {


Same comments as above for ttl's param documentation and optional nature.

shubham1172 · 2022-03-16T12:56:18Z

test/actor/DemoActorReminder2Impl.ts


  async init(): Promise<string> {
-    await super.registerActorReminder("my-reminder-name", Temporal.Duration.from({ seconds: 2 }), Temporal.Duration.from({ seconds: 1 }), 123);
+    await super.registerActorReminder("my-reminder-name", Temporal.Duration.from({ seconds: 2 }), Temporal.Duration.from({ seconds: 2 }), Temporal.Duration.from({ seconds: 1 }), 123);


nit: line break in params for readability

shubham1172 · 2022-03-16T12:56:22Z

test/actor/DemoActorReminderImpl.ts


  async init(): Promise<string> {
-    await super.registerActorReminder("my-reminder-name", Temporal.Duration.from({ seconds: 2 }), Temporal.Duration.from({ seconds: 1 }), 123);
+    await super.registerActorReminder("my-reminder-name", Temporal.Duration.from({ seconds: 2 }), Temporal.Duration.from({ seconds: 2 }), Temporal.Duration.from({ seconds: 1 }), 123);


nit: line break in params for readability

shubham1172 · 2022-03-16T12:56:25Z

test/actor/DemoActorTimerImpl.ts


  async init(): Promise<string> {
-    await super.registerActorTimer("my-timer-name", "countBy", Temporal.Duration.from({ seconds: 2 }), Temporal.Duration.from({ seconds: 1 }), 100);
+    await super.registerActorTimer("my-timer-name", "countBy", Temporal.Duration.from({ seconds: 2 }), Temporal.Duration.from({ seconds: 2 }), Temporal.Duration.from({ seconds: 1 }), 100);


nit: line break in params for readability

shubham1172

LGTM @amulyavarote, only had a few nits.

Signed-off-by: Amulya Varote <amulyavarote@QTM-SWATHIKIL-1.redmond.corp.microsoft.com> Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>

Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>

amulyavarote requested review from a team as code owners March 14, 2022 19:40

shubham1172 reviewed Mar 16, 2022

View reviewed changes

shubham1172 suggested changes Mar 16, 2022

View reviewed changes

amulyavarote force-pushed the feature/actors_reminder_ttl branch 2 times, most recently from ea7f558 to 2373715 Compare April 14, 2022 00:18

amulyavarote and others added 9 commits April 19, 2022 10:36

Added ttl param to actor timer and reminder

651d4a0

Signed-off-by: Amulya Varote <amulyavarote@QTM-SWATHIKIL-1.redmond.corp.microsoft.com> Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>

Changed from TTL to ttl

eb23da9

Signed-off-by: Amulya Varote <amulyavarote@QTM-SWATHIKIL-1.redmond.corp.microsoft.com> Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>

Fixed test case

f6b86c4

Signed-off-by: Amulya Varote <amulyavarote@QTM-SWATHIKIL-1.redmond.corp.microsoft.com> Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>

Reverted the test file

ec7fab9

Signed-off-by: Amulya Varote <amulyavarote@QTM-SWATHIKIL-1.redmond.corp.microsoft.com> Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>

Removed last line

1a8dff0

Signed-off-by: Amulya Varote <amulyavarote@QTM-SWATHIKIL-1.redmond.corp.microsoft.com> Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>

Added test cases

0bcd2e8

Signed-off-by: Amulya Varote <amulyavarote@QTM-SWATHIKIL-1.redmond.corp.microsoft.com> Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>

Modified test cases for actors ttl

e6c86f8

Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>

Fixed e2e tests for actor ttl

a013a69

Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>

Added a few scenarios to the test case

3f08897

Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>

amulyavarote force-pushed the feature/actors_reminder_ttl branch from 2373715 to 3f08897 Compare April 19, 2022 17:43

XavierGeerinck merged commit eb6ee2e into dapr:master Apr 19, 2022

amulyavarote deleted the feature/actors_reminder_ttl branch April 19, 2022 18:18

shubham1172 mentioned this pull request Apr 29, 2022

REQUEST: Approver status for @shubham1172 in dapr/js-sdk dapr/community#171

Closed

4 tasks

amulyavarote mentioned this pull request Jul 13, 2022

REQUEST: New approver request for amulyavarote dapr/community#190

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Added TTL param to actor timer and reminder #212

Added TTL param to actor timer and reminder #212

Uh oh!

amulyavarote commented Mar 14, 2022 •

edited

Loading

Uh oh!

codecov bot commented Mar 14, 2022 •

edited

Loading

Uh oh!

XavierGeerinck commented Mar 15, 2022

Uh oh!

shubham1172 Mar 16, 2022

Uh oh!

shubham1172 Mar 16, 2022

Uh oh!

shubham1172 Mar 16, 2022

Uh oh!

shubham1172 Mar 16, 2022

Uh oh!

shubham1172 Mar 16, 2022

Uh oh!

shubham1172 Mar 16, 2022

Uh oh!

shubham1172 Mar 16, 2022

Uh oh!

shubham1172 Mar 16, 2022

Uh oh!

shubham1172 Mar 16, 2022

Uh oh!

shubham1172 Mar 16, 2022

Uh oh!

shubham1172 left a comment

Uh oh!

Uh oh!

	ttl?: Temporal.Duration.from({ seconds: 1 }),
	ttl: Temporal.Duration.from({ seconds: 1 }),

	* @param ttl time to live
	* @param ttl time duration after which the reminder will be expired and deleted

Added TTL param to actor timer and reminder #212

Added TTL param to actor timer and reminder #212

Uh oh!

Conversation

amulyavarote commented Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue reference

Checklist

Uh oh!

codecov bot commented Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

XavierGeerinck commented Mar 15, 2022

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!

shubham1172 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amulyavarote commented Mar 14, 2022 •

edited

Loading

codecov bot commented Mar 14, 2022 •

edited

Loading