-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Update DeviceGroupQuery.cs #3221
Conversation
Fixed timing of the first change of behaviour, moved from OnReceive to Constructor method
Thanks @nsanitate ! |
@Aaronontheweb you can try the patch cloning https://github.com/nsanitate/iot-app and starting tests. If you follow the tutorial changing behaviour in OnResolve method instead of in Constructor, all tests in DeviceGroupQueryTest class will fail. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's some smell in this tutorial...
@@ -26,6 +27,8 @@ public DeviceGroupQuery(Dictionary<IActorRef, string> actorToDeviceId, long requ | |||
Timeout = timeout; | |||
|
|||
queryTimeoutTimer = Context.System.Scheduler.ScheduleTellOnceCancelable(timeout, Self, CollectionTimeout.Instance, Self); | |||
|
|||
Become(WaitingForReplies(new Dictionary<string, ITemperatureReading>(), new HashSet<IActorRef>(ActorToDeviceId.Keys))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just change this to WaitingForReplies
without the Become
syntax here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output of WaitingForReplies
is a factory method that will be used as input of Context.Become
method. The author use it in the same way at row 116.
#region query-state | ||
protected override void OnReceive(object message) | ||
{ | ||
Become(WaitingForReplies(new Dictionary<string, ITemperatureReading>(), new HashSet<IActorRef>(ActorToDeviceId.Keys))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is wrong. Good catch.
#endregion | ||
#endregion | ||
|
||
protected override void OnReceive(object message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably just refactor this actor to do all of its actual message handling inside OnReceive
instead of having this behavior-switching. We could just make the state private members. Although this is a cool way of showing how you can use closures to build receive methods, which is a valid technique in practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the purpose of this tutorial was to teach using the Context.Become() trick instead of just making the repliesSoFar and stillWaiting structures mutable fields of the actor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. In that case, we should leave it as-is. Want to teach the concept.
@nsanitate what do you think of my comments? |
Fixed exit condition check on stillWaiting instead of repliesSoFar
Fixed timing of the first change of behaviour, moved from OnReceive to Constructor method