Page MenuHomeElementl

[sensors-4] add sensors to scheduler event loop

Authored by prha on Nov 3 2020, 10:04 PM.



1st pass at evaluating sensors in the schedule event loop.

There's a fair amount of code duplication, but can start combining if this ends up being the right approach.

This doesn't allow for customization of the evaluation interval or provide for threading to mitigate for latency from one sensor evaluation to affect other sensors / schedules.

Test Plan


Diff Detail

R1 dagster
Lint Not Applicable
Tests Not Applicable

Event Timeline

prha requested review of this revision.Nov 3 2020, 10:21 PM
dgibson requested changes to this revision.Nov 4 2020, 3:06 AM

this seems reasonable, time for tests?


update copy


this will be using external repository origin to work if you make the changes from the previous diff so I won't belabor that


setting the timestamp here doesn't mess up future checks if it fails does it? like if it suddenly crashed after doing this write and started up again, would it be idempotent?

If it would that's probably undesirable and maybe we should filter be completed ticks when determining the checkpoint time?

This revision now requires changes to proceed.Nov 4 2020, 3:06 AM
prha marked 2 inline comments as done.


will keep working through the stack - seems like this sensor format is not the final version since it just returns a boolean? and isn't currently offering any idempotency/run-exactly-once guarantees right?

109–116 ↗(On Diff #25281)

this is going to need a rebase as this method no longer exists- should probably be a new SensorDaemon in the dagster daemon loop in dagster/daemon/ Should be functionally the same though


Probably worth chatting about this part IRL, having some trouble totally following it - seems hard to guarantee idempotence/exactly-one-run-per-sensor-execution for? what if it fails in the middle of doing this?

Do we want to maintain the idempotence guarantees for sensors?


i had assumed the cursor would be based on the execution keys of the created runs, not just a string version of the current time? For idempotence purposes?


are we eventually going to include the cursor here? seems like it would need to be part of this in order for it to be useful?


this formatting is going to be updated right?



The scaffolding here seems fine, we will need to sort out the exact details of the recovery-from-failure rules but it seems like that is happening later in the stack where we will actually use the execution_key and cursor params?

Will have a bit of a painful rebase though

This revision is now accepted and ready to land.Nov 17 2020, 3:10 PM
This revision was landed with ongoing or failed builds.Nov 23 2020, 3:49 PM
This revision was automatically updated to reflect the committed changes.