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.

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

[WIP] Add support for TTL and time/interval formats in actor timers/reminders #652 #658

Draft
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

Giovds
Copy link

@Giovds Giovds commented Nov 22, 2021

Description

This PR should add the following items:

ActorTimers

  • Add TTL option
  • Allow for repetitions in TTL and Period with ISO-8601 format

ActorReminders

  • Add TTL option
  • Allow for repetitions in TTL and Period with ISO-8601 format

Integration test

  • Add integration test validating the added features

Issue reference

The issue this PR will close: #652

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

@ghost
Copy link

ghost commented Nov 22, 2021

CLA assistant check
All CLA requirements met.

@Giovds Giovds force-pushed the issue-652-add-ttl-for-timers-and-reminders branch from 8d55d05 to 2285239 Compare November 25, 2021 22:59
@mukundansundar
Copy link
Contributor

@Giovds Is there any progress in this?

@Giovds
Copy link
Author

Giovds commented Jan 5, 2022

@mukundansundar I haven't had the time to finish this yet, and I will still be unavailable next week. I expect to have time again afterwards. I think what is left is to write integration tests to prove that it works, but I couldn't get them to run them locally yet.

@Giovds Giovds force-pushed the issue-652-add-ttl-for-timers-and-reminders branch from e63203d to 76945aa Compare January 26, 2022 21:20
Signed-off-by: Giovanni van der Schelde <[email protected]>
Signed-off-by: Giovanni van der Schelde <[email protected]>
Signed-off-by: Giovanni van der Schelde <[email protected]>
Signed-off-by: Giovanni van der Schelde <[email protected]>
Signed-off-by: Giovanni van der Schelde <[email protected]>
Signed-off-by: Giovanni van der Schelde <[email protected]>
There is no need to create an instance of the DurationUtils or to extend
it.

Signed-off-by: Giovanni van der Schelde <[email protected]>
Signed-off-by: Giovanni van der Schelde <[email protected]>
Signed-off-by: Giovanni van der Schelde <[email protected]>
Signed-off-by: Giovanni van der Schelde <[email protected]>
Signed-off-by: Giovanni van der Schelde <[email protected]>
Signed-off-by: Giovanni van der Schelde <[email protected]>
Since the class is package-private there is no need to make the getters
public.

Signed-off-by: Giovanni van der Schelde <[email protected]>
Signed-off-by: Giovanni van der Schelde <[email protected]>
Signed-off-by: Giovanni van der Schelde <[email protected]>
Signed-off-by: Giovanni van der Schelde <[email protected]>
Signed-off-by: Giovanni van der Schelde <[email protected]>
Signed-off-by: Giovanni van der Schelde <[email protected]>
@mukundansundar
Copy link
Contributor

@Giovds nudge - is there any update on this PR ?

@codecov
Copy link

codecov bot commented Mar 15, 2022

Codecov Report

Merging #658 (cf60e2d) into master (eb215fb) will increase coverage by 0.47%.
The diff coverage is 97.65%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #658      +/-   ##
============================================
+ Coverage     77.62%   78.09%   +0.47%     
- Complexity      993     1028      +35     
============================================
  Files            91       92       +1     
  Lines          3128     3196      +68     
  Branches        342      349       +7     
============================================
+ Hits           2428     2496      +68     
- Misses          534      535       +1     
+ Partials        166      165       -1     
Impacted Files Coverage Δ
.../io/dapr/actors/runtime/ActorObjectSerializer.java 83.83% <85.00%> (-0.87%) ⬇️
...ain/java/io/dapr/actors/runtime/AbstractActor.java 88.46% <100.00%> (+3.12%) ⬆️
...va/io/dapr/actors/runtime/ActorReminderParams.java 100.00% <100.00%> (ø)
.../java/io/dapr/actors/runtime/ActorTimerParams.java 100.00% <100.00%> (ø)
...in/java/io/dapr/actors/runtime/DaprGrpcClient.java 95.60% <100.00%> (+0.09%) ⬆️
sdk/src/main/java/io/dapr/utils/DurationUtils.java 98.43% <100.00%> (+2.78%) ⬆️
.../src/main/java/io/dapr/utils/RepeatedDuration.java 100.00% <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 eb215fb...cf60e2d. Read the comment docs.

@Giovds
Copy link
Author

Giovds commented Mar 15, 2022

@Giovds nudge - is there any update on this PR ?

@mukundansundar #652 (comment)

Mainly stuck at creating integration tests and running them locally.

@cicoyle
Copy link
Contributor

cicoyle commented Dec 21, 2023

gentle ping - @Giovds. Please see README as to how to run the tests locally

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add support for TTL and time/interval formats in actor timers/reminders
3 participants