Page MenuHomePhabricator

RFC: populate last_completed_time with the sensor state update time
ClosedPublic

Authored by prha on Dec 1 2020, 4:30 PM.

Details

Summary

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?

Test Plan

bk

Diff Detail

Repository
R1 dagster
Branch
prha/sensor_last_time
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

prha requested review of this revision.Dec 1 2020, 4:47 PM
prha retitled this revision from populate last_completed_time with the sensor state update time to RFC: populate last_completed_time with the sensor state update time.Dec 1 2020, 4:52 PM
prha edited the summary of this revision. (Show Details)
prha added reviewers: dgibson, schrockn.
schrockn requested changes to this revision.Dec 1 2020, 6:49 PM

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?

This revision now requires changes to proceed.Dec 1 2020, 6:49 PM

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?

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

prha edited the summary of this revision. (Show Details)

rebase, rename last_completed_timestamp => last_tick_timestamp (requires wipe)

this makes much more sense to me

This revision is now accepted and ready to land.Dec 4 2020, 10:08 PM
This revision was landed with ongoing or failed builds.Dec 7 2020, 5:03 PM
This revision was automatically updated to reflect the committed changes.