Pastdue #195
Pastdue #195
Conversation
|
Also now addresses Azure/azure-functions-host#1246. |
| // Track the time that was used to create 'expectedNextOccurrence'. | ||
| DateTime lastUpdated; | ||
|
|
||
| if (lastStatus.LastUpdated != default(DateTime)) |
There was a problem hiding this comment.
nit: could collapse this whole if else to something like:
DateTime lastUpdated = lastStatus?.LastUpdated ?? lastStatus?.Last ?? now;
expectedNextOccurrence = schedule.GetNextOccurrence(lastUpdated);
I like the comments, def helps the grok.
There was a problem hiding this comment.
I agree -- but this is really confusing when you come back to it several months later (like always happens :-)), so I wanted to make sure the code and comments were verbose enough to be understood.
| // if the schedule has changed and the next occurrence is in the past, | ||
| // recalculate it based on the current time as we don't want it to register | ||
| // immediately as 'past due'. | ||
| if (now > expectedNextOccurrence) |
There was a problem hiding this comment.
Just to be clear, we are treating 'now' as if it is the exact moment of schedule change, correct?
nit: Maybe we could route this via a function call to consolidate this to a invalid/no last status codepath, as this moment basically says 'throw out everything we know and start over'.
There was a problem hiding this comment.
May want to change CheckPastDueAsync comments above to make sure they're consistent with this new delay behavior
There was a problem hiding this comment.
Yes, think of LastUpdated as "the time we used when we last calculated Next". It's real value is allowing you to look at the current schedule and recalculate 'Next' to see if it has changed. We don't trust 'Next' unless we recalculate it ourselves on startup with the current schedule.
Does that make sense? I know it can get confusing -- hence the verbose comments :-)
| } | ||
| else | ||
| else if (lastStatus.Last != default(DateTime)) |
There was a problem hiding this comment.
Shouldn't use of Last if present take precedence over use of the new LastUpdated? It seems we should calculate next using Last, LastUpdate, Now, in that order.
There was a problem hiding this comment.
If LastUpdated exists, it will always be the same as Last. I'll look and see if I can easily swap them around and still have it be readable (I think I can).
|
Shipit |
…hecule if changed and already past due
Addressing #194.
The problem is that if the function has never run, we have no
Lastvalue to use for calculatingNextwhen the host starts. So instead, we calculate fromnow. If the timer is past due, that means we'll miss it. Some customers have seen this happen when their site wakes up and restarts for one reason or another -- the timer doesn't get around to evaluating until a couple seconds after the scheduled execution. At that point, we recalculate the next execution and don't know that we've missed one.This change adds a
LastUpdatedvalue to our serialized status. Even if the function has never run, we'll updateLastUpdatedto be the last time that we used when calculatingNext. So when the host restarts, we have something to work with.I've added tests to make sure that the 'legacy' behavior still works as expected -- since no one will have a
LastUpdatedvalue when this code starts running.