Start using InstigationTick.scheduledExecutionTimestamp when rendering the schedule timeline #26823
Conversation
gibsondan
force-pushed
the
consecutivefailurecount3
branch
from
ac79a45 to
534f6a6
Compare
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 8a034bd. |
gibsondan
force-pushed
the
consecutivefailurecount3
branch
from
534f6a6 to
03ddbad
Compare
gibsondan
force-pushed
the
consecutivefailurecount3
branch
from
03ddbad to
8a8cb74
Compare
gibsondan
force-pushed
the
consecutivefailurecount2
branch
from
ae401a6 to
55088ae
Compare
…g the schedule timeline Summary: Right now schedule ticks smush two separate concepts into the 'timestamp' field: the time the schedule tick was supposed to run, and the time it actually ran. This means that if the tick fails and retries, the retry overwrites the original tick in a confusing way, and also means that if a tick happens way after its scheduled time for whatever reason (e..g the daemon was down) the resulting duration is much longer than it should have been. The plan is to alleviate those issues by moving the 'timestamp' field to be the itme that the tick actually started, and add a new 'scheduledExecutionTimestamp' field for when you when you want to know when it was supposed to happen. This PR adds the new "scheduledExecutionTimestamp" field to the tick object and uses it where appropriate to keep the existing behavior. I think more substantial design changes will likely need to be made to the schedule timeline view to display both timestamps where appropriate, but wanted to send this out to seed that discussion.
gibsondan
force-pushed
the
consecutivefailurecount3
branch
from
8a8cb74 to
8a034bd
Compare
| @@ -133,7 +133,7 @@ const TickDetailsDialogImpl = ({tickId, tickResultType, instigationSelector}: In | |||
| <> | |||
| <span>Tick for {instigationSelector.name}: </span> | |||
| <TimestampDisplay | |||
| timestamp={tick.timestamp} | |||
| timestamp={tick.scheduledExecutionTimestamp ?? tick.timestamp} | |||
There was a problem hiding this comment.
This all looks good but I want to sanity check the ordering here, I'd assumed from the PR description that the old behavior was the inverse of this -- the time it actually executed, if it did execute, and the time it is scheduled for, if it did not yet execute. If that's the case I think these would be tick.timestamp ?? tick.scheduledExecutionTimestamp, since tick.scheduledExecutionTimestamp is the one that is always present?
There was a problem hiding this comment.
edit: Ahh I see, maybe this is falling back to tick.timestamp for back-compat so this is more of a "new version ?? old version". Carry on!
Summary:
Right now schedule ticks smush two separate concepts into the 'timestamp' field: the time the schedule tick was supposed to run, and the time it actually ran. This means that if the tick fails and retries, the retry overwrites the original tick in a confusing way, and also means that if a tick happens way after its scheduled time for whatever reason (e..g the daemon was down) the resulting duration is much longer than it should have been.
The plan is to alleviate those issues by moving the 'timestamp' field to be the itme that the tick actually started, and add a new 'scheduledExecutionTimestamp' field for when you when you want to know when it was supposed to happen.
This PR adds the new "scheduledExecutionTimestamp" field to the tick object and uses it where appropriate to keep the existing behavior. I think more substantial design changes will likely need to be made to the schedule timeline view to display both timestamps where appropriate, but wanted to send this out to seed that discussion.
Summary & Motivation
How I Tested These Changes
Changelog