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

Timers for self scheduled messages added, FSM timers fixes #3778

Merged
merged 11 commits into from
Mar 25, 2020

Conversation

zbynek001
Copy link
Contributor

migrated from akka/akka#22873

@ismaelhamed
Copy link
Member

@zbynek001 Nice!! I ported just the fix a while back (#3313) but the rest of that PR has been in my backlog since then.

Copy link
Contributor

@Horusiath Horusiath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job 👍 It's a big task to complete. My main concern is related to narrowing original interface (which uses object keys) only to accept string keys. There are valid options where users may want to use other types as keys as well.

@ismaelhamed
Copy link
Member

@zbynek001 @Aaronontheweb any news on this one?

Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need some API changes

We also need to add some docs to describe how to send recurring messages using this interface here:

https://github.com/akkadotnet/akka.net/tree/dev/docs/articles/actors

I'd recommend adding a new article called "recurring and timed messages"

@Aaronontheweb Aaronontheweb added this to the 1.4.3 milestone Mar 18, 2020
@Aaronontheweb
Copy link
Member

@zbynek001 updated the review - I think this needs some minor API changes and some documentation, but otherwise looks like a great improvement.

@Aaronontheweb Aaronontheweb modified the milestones: 1.4.3, 1.4.4 Mar 18, 2020
@zbynek001
Copy link
Contributor Author

@Aaronontheweb Should it be done via the Actor producer pipeline or directly in ActorCell.CreateNewActorInstance()?
Or some other way, when the Actor producer pipeline is marked as obsolete?

@Aaronontheweb
Copy link
Member

@zbynek001 I was going to suggest that, but ultimately deleted it. I think that's probably a better way to go though - have it be done via the way actors are created. I think that'd be a good compromise for now. We're going to have to replace that pipeline with something else eventually and I get the feeling that default interfaces probably aren't going to be it.

@zbynek001
Copy link
Contributor Author

at the end had to initialize Timers in ActorBase constructor. Because often you want to configure them in your constructor

@zbynek001
Copy link
Contributor Author

this should be ready. I guess the failed test is unrelated to this pr and at least locally it's all passing

@zbynek001 zbynek001 mentioned this pull request Mar 24, 2020
Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -105,6 +103,10 @@ protected ActorBase()
{
if (ActorCell.Current == null)
throw new ActorInitializationException("Do not create actors using 'new', always create them using an ActorContext/System");

if (this is IWithTimers withTimers)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -92,7 +91,6 @@ public interface IInternalActor
public abstract partial class ActorBase : IInternalActor
{
private IActorRef _clearedSelf;
private Scheduler.TimerScheduler timers;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good

@Aaronontheweb
Copy link
Member

We're going to need to document these changes - for the time being I've created an open issue here: #4357

@Aaronontheweb Aaronontheweb merged commit c983b33 into akkadotnet:dev Mar 25, 2020
This was referenced Mar 31, 2020
@zbynek001 zbynek001 deleted the timers branch July 16, 2021 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants