Differential D5342
RFC: populate last_completed_time with the sensor state update time Authored by prha on Dec 1 2020, 4:30 PM. Tags None Subscribers None
Details If the sensor changes state (on/off), then when it is flipped on, we should only consider the time ranges in which it has been on. We should overwrite the last_completed_time field that is passed through to the context. We should also consider renaming last_completed_time to something else... maybe just since_time? bk
Diff Detail
Event TimelineComment Actions I have a higher level question. Should it be last_completed_time? Don't we want the common case to be last_run_time to incorporate running runs? Comment Actions last_run_time seems like it can be confused with when the sensor last kicked off a run. We want to include the last time the sensor was evaluated, even if it ended up skipping. I used to have this as last_evaluation_time - we could switch it back to that. But this diff is introducing a different concept... what is the latest time range which the sensor should consider? This diff is arguing that it should be the max(time_since_sensor_enabled, time_since_sensor_last_evaluated). What should that thing be called? Comment Actions separate from nick's question - this looks right conceptually but I think we should do it lower in the stack like we do for schedules? And probably only when the sensor is started, not when it is stopped? Here's the corresponding code from schedules we could mimic: https://sourcegraph.com/github.com/dagster-io/dagster/-/blob/python_modules/dagster/dagster/core/scheduler/scheduler.py#L205 |