Inconsistent behaviour between docs and implementation in Actor Timers and Reminders

[Giovds]

While implementing #658 (Adding support for TTL and intervals) I possibly came across some inconsistency with the documentation on Actor Timers and Reminders.

Expected Behavior

The dapr-sdk-actors implementation - the AbstractActor class which a client inherits for their own Actor implementation, to be more specific - matches the documentation. Or of course, vice-versa, the documentation matches the implementation.

Actual Behavior

The current implementation of registerReminder() and registerActorTimer() in AbstractActor requires both dueTime and period to be present.

I've found the following documentation on Actor Timers: https://docs.dapr.io/developing-applications/building-blocks/actors/howto-actors/#actor-timers-and-reminders

Which states the following:

dueTime is an optional parameter that sets time at which or time interval before the callback is invoked for the first time.

period is an optional parameter that sets time interval between two consecutive callback invocations

The request structure for reminders is identical to those of actors. Please refer to the actor timers examples.

The reference API documentation states the following on Reminders: https://docs.dapr.io/reference/api/actors_api/#create-actor-reminder. I can't seem to find any information about optional fields here, or TTL for that matter.

dueTime | Specifies the time after which the reminder is invoked, its format should be time.ParseDuration format period | Specifies the period between different invocations, its format should be time.ParseDuration format or ISO 8601 duration format with optional recurrence.

[mukundansundar]

The lines in the actor implementation in Dapr. seem to make that dueTime is indeed optional ... https://github.com/dapr/dapr/blob/master/pkg/actors/actors.go#L1009-L1015 Wherein when due time is not given, now is taken as a the value.

@artursouza I think this might be something that would need to change in Java SDK ... Any thoughts ?

[artursouza]

Yes, the Java SDK should match the expectation in runtime.

[mukundansundar]

My proposal is add new method signatures that does not require optional parameters. And keep the existing method as such. This will not break existing users of this feature.

[mukundansundar]

@Giovds Will you be interested in working on this?

[shubham1172]

FYI, the .NET SDK Actor.cs handles dueTime and period without default values like the following:

        // ...
        /// <param name="dueTime">The amount of time to delay before the async callback is first invoked.
        /// Specify negative one (-1) milliseconds to prevent the timer from starting.
        /// Specify zero (0) to start the timer immediately.
        /// </param>
        /// <param name="period">
        /// The time interval between invocations of the async callback.
        /// Specify negative one (-1) milliseconds to disable periodic signaling.</param>
        // ...
        public async Task<ActorTimer> RegisterTimerAsync(
            string timerName,
            string callback,
            byte[] callbackParams,
            TimeSpan dueTime,
            TimeSpan period)

If we go for method overloading, we won't be able to support the case above where dueTime is -1, i.e. timer never starts. However, this functionality is not documented here https://docs.dapr.io/reference/api/actors_api/#create-actor-reminder so we might skip it?

Also for method overloading, since both dueTime and period have the same type Duration, how do we differentiate between the two?

1. keeping dueTime and omitting period
2. omitting dueTime and keeping period

An option here can be to always have dueTime in parameters and explicitly ask the caller to provide Duration.ZERO if it is to be triggered immediately.

Another option here can be to use something similar to a builder pattern:

        new Reminder(name, state) 
        .withDueTime(dueTime) // where omitting this means that it will be instantly triggered
        .withPeriod(period) // where omitting this means that there will be no repetition
        .register(abstractActor)

[mukundansundar]

I like the option of having the user explicitly setting the value to Zero. This makes sure that the user knows that he is setting it to zero and also we do not have an implicit value and behaviour which is only available in the documentation.

[mukundansundar]

//cc @artursouza Any comments?

[artursouza]

I agree with the builder pattern proposed above.

[Giovds]

@Giovds Will you be interested in working on this?

@mukundansundar I am currently working on some other stuff as well, but I might give it a go if it is still an open issue by the time I am done. In the meantime, if anyone wants to give it a go feel free to do so - might even be a decent up-for-grabs.

[addjuarez]

/assign

[artursouza]

We need to revisit this issue before we can merge the PR. Also, how to support the ISO format too.