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

One minute minimum restriction on Reminder period #4218

Closed
emilevr opened this issue Mar 14, 2018 · 12 comments · Fixed by #6854
Closed

One minute minimum restriction on Reminder period #4218

emilevr opened this issue Mar 14, 2018 · 12 comments · Fixed by #6854
Labels
Milestone

Comments

@emilevr
Copy link

emilevr commented Mar 14, 2018

It would be useful to be able to set the minimum Reminder period to less than one minute, for a handful of critical grains, that may not receive messages often and that we don't want to leave deactivated for up to a minute.

One example would be a grain that primarily runs a timer to perform some critical work, that should ideally always be performed at intervals below a minute.

If it was possible to set the reminder to less than one minute, then the timer might not even be necessary, if the reminder period was sufficient.

I am guessing that the one minute restriction is performance related. If so, in my opinion, the decision should be left to the application designer...

@miker1423
Copy link
Contributor

Hello, answering your question, if your period is less than a minute, then you can use a timer in combination with a reminder as described at the docs.

@emilevr
Copy link
Author

emilevr commented Mar 14, 2018

Thanks for the response. I am more interested here in why the restriction exists. Without the restriction, and all other considerations aside, there would be no need to combine reminders and timers in the first place, if the desired behaviour was re-activation of the grain.

The fact that there is a restriction indicates that there is some sort of concern/constraint w.r.t. reminders with short periods, but it isn't clear to me what those constraints are. Perhaps it is performance related, as having tens of thousands on reminders with very short periods would add quite a bit of base load, however I can also see the usefulness of being able to use short periods for a handful of critical grains.

An example would be a grain that is used to implement a system wide tick, perhaps via pushing a message into a stream, or some similar mechanism. Let's say this tick should happen every 10 seconds. Using a reminder for this would seem to be a good choice, as the single grain that pushes out the ticks would need to be reactivated if necessary. With the current restriction we would have to wait for up to a minute before the 10 second ticks would resume, in cases where the grain was deactivated, for whatever reason.

PS. @ReubenBond suggested I list this as a separate issue for discussion, so I assume there is something to discuss. :-)

@sergeybykov
Copy link
Contributor

Reminders are much more expensive than timers - they generally involve a remote call with serialization and messaging overhead of that. The limit is in place to protect developers from accidentally overloading silos when too many grains try to register reminders firing too frequently.

@sergeybykov sergeybykov added this to the Triage milestone Mar 14, 2018
@emilevr
Copy link
Author

emilevr commented Mar 16, 2018

May I suggest that a mechanism be added to opt out of this behaviour, or override the minimum value, via config? Perhaps a global config setting or alternatively a per grain type setting similar to how the collection age limits are configured.

Currently, should a requirement exist for some handful of grains to be kept active at a period lower than one minute, some external service would need to be used or perhaps a per silo standard timer, which would be far more inefficient and certainly less elegant.

@shlomiw
Copy link
Contributor

shlomiw commented Apr 1, 2018

This restriction is also problematic in Tests, where you don't care about scalability and you want to check the reminders logic without waiting the full minute.

I've seen what you've done in Orleans internal tests with ReminderTestGrain2.UnvalidatedReminderRegistry to override this restriction but I find it cumbersome to use it in the grain code just to test my grains.

I think it should be optional as @emilevr suggested.

FYI - for now I've overridden Orleans.Core.Constants.MinReminderPeriod in my tests using reflection (ouch!).

@sergeybykov sergeybykov modified the milestones: Triage, Backlog Apr 2, 2018
@pentp
Copy link
Contributor

pentp commented Aug 29, 2018

I would also like to use a smaller reminder period for tests. I tried the suggestion to use timers in combination with reminders, but that is problematic because timers don't have the same re-entrancy guarantees.

At least the minimum period constant should be moved from ReminderRegistry to the reminder service so that local in-memory reminders can have a different minimum from expensive remote reminder services.

Probably a separate issue: there should be an option for timers that respect re-entrancy rules like reminders do.

@sergeybykov
Copy link
Contributor

At least the minimum period constant should be moved from ReminderRegistry to the reminder service so that local in-memory reminders can have a different minimum from expensive remote reminder services.

I'm not sure I understand the distinction here between local and remote. A partition of reminder service in a silo sends reminder messages to grains that are activated in other silos. So there is no locality here really.

I think maybe a better approach for tests is to inject a test implementation of the reminder service, to separate test concerns from production ones.

Probably a separate issue: there should be an option for timers that respect re-entrancy rules like reminders do.

#2574

@ChaosOrb781
Copy link

I think @emilevr has a point, our university introduced a new course where orleans is a basis for some of the assignments with very tight deadlines, having to perform local tests (as in within router network) using a TestCluster instance is simply just too time consuming as some of the requirements are to utilize the reminders to say, keep a ball in play between players where they can decide to hold the ball or torse it after x time. Another requirement is to ensure persistence of the grain states, so we can ensure however many balls are in play can be observed at any time and ensure the amount initially spawned into the game is consistant

@sergeybykov sergeybykov modified the milestones: Backlog, 4.0.0 Dec 31, 2019
@tomerpeled
Copy link

This restriction is also problematic in Tests, where you don't care about scalability and you want to check the reminders logic without waiting the full minute.

I've seen what you've done in Orleans internal tests with ReminderTestGrain2.UnvalidatedReminderRegistry to override this restriction but I find it cumbersome to use it in the grain code just to test my grains.

I think it should be optional as @emilevr suggested.

FYI - for now I've overridden Orleans.Core.Constants.MinReminderPeriod in my tests using reflection (ouch!).

BTW the above approach doesn't work anymore starting from .NET Core 3.0, since it is a readonly field and in .NET Core 3.0+ they started to enforce it :(
You'll get the following error:
System.FieldAccessException: Cannot set initonly static field 'MinReminderPeriod' after type 'Orleans.Runtime.Constants' is initialized.

@mllab-nl
Copy link

mllab-nl commented Jun 12, 2020

For Testing purposes you can use UnvalidatedReminderRegistry from here
https://github.com/dotnet/orleans/blob/master/test/Grains/TestInternalGrains/ReminderTestGrain2.cs

@pentp
Copy link
Contributor

pentp commented Jun 13, 2020

The 1 minute check is done before a reminder even reaches any IReminderRegistry implementations. That unit testing example uses the registry directly (not through Grain functions).

The 1 minute restriction as a default is fine, but then it should be configurable.

@sergeybykov
Copy link
Contributor

We support making the minimum reminder period configurable, so long as a warning is logged upon silo startup that high-frequency reminders are dangerous for production, if the minimum is set below 1 minute. A PR would be very welcome.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants