UnregisterReminder takes IGrainReminder but actually requires ReminderData which is internal

[juho-hanhimaki]

The UnregisterReminder takes IGrainReminder but actually requires ReminderData.

orleans/src/Orleans.Reminders/ReminderService/LocalReminderService.cs

Line 155 in c846cb0

var remData = (ReminderData)reminder;

This is inconvenient as the ReminderData type is internal and only acquirable via GetReminder which involves often unnecessary database(IReminderTable) call. I propose either to make ReminderData type public or changing the API and the implementation in such way that consumers of the API are able to construct the type required by the method manually.

[juho-hanhimaki]

Basically being able to do either of these would be great:

    public async Task ReceiveReminder(string reminderName, TickStatus status)
    {
        await this.UnregisterReminder(reminderName);
        // or
        await this.UnregisterReminder(new ReminderData(this.GetGrainId(), reminderName, null));
    }

[ReubenBond]

@juho-hanhimaki it sounds like this issue is an API proposal, eg, to have an UnregisterReminder call which accepts a string (reminderName) instead of ReminderData. Is that correct?

I imagine that the API is designed this way for consistency reasons, but I could be wrong. Still, I think we should consider this.

[juho-hanhimaki]

@ReubenBond it doesn't really matter to me whether API is improved or ReminderData is made public. I would be happy as long as it is possible to unregister a reminder without unnecessary GetReminder call.

That said improving the API would certainly be much cleaner way to achieve this.

I would say the current API design is not great. I did try implementing the IGrainReminder interface and passing it to the UnregisterReminder in hopes of unregistering a reminder without first doing GetReminder. You can imagine I was quite surprised to find out that instead of IGrainReminder, an internal type ReminderData was required, just not in the method signature.

[christallire]

We could improve the "ReceiveReminder" api by adding IGrainReminder reminder like:

Task ReceiveReminder(IGrainReminder reminder, string reminderName, TickStatus status)

or by simplifying

Task ReceiveReminder(IGrainReminder reminder, TickStatus status)

because in most cases unregistering requires redundant await this.Getreminder(reminderName) in the code
Do we really need another "await" for the unregistering reminder?

[juho-hanhimaki]

I like that too.