Page MenuHomePhabricator

[run-queue-5] Default runs coordinator (sends to external process)

Authored by johann on Oct 27 2020, 2:41 AM.



Adds the interface to the external runs coordinator

Test Plan


Diff Detail

R1 dagster
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

change from MockedRunLauncher to MockedRunQueuer

johann retitled this revision from [run-queue-5] Default run queuer (sends to external process) to [run-queue-5] Default runs coordinator (sends to external process).Oct 29 2020, 8:26 PM
johann edited the summary of this revision. (Show Details)
johann added reviewers: alangenfeld, dgibson.
dgibson requested changes to this revision.Oct 30 2020, 1:41 PM

other than the name this seems good to me, see inline. We can also do a single diff at the end of this stack where we do a big codemod to make all the final naming decisions, since we're going back and forth so much and renaming through a stack is a pain

14 ↗(On Diff #24841)

wait is this the default? Isn't the one that launches immediately the default? The one where things 'just work' without extra deployment and config should be the default IMO (and the one that we're setting as the default in the instance)

This revision now requires changes to proceed.Oct 30 2020, 1:41 PM

few small things inline


this could go on the base class too?


realizing now that this is mildly confusing (even though it also exists on the runlauncher) since there is a separate InstanceRef class


opt_inst_param doesn't seem right here even if this run coordinator happens to not use it


should this be a static pipeline_enqueued method on DagsterEvent like the other event creation methods?


seeing it in action now there's something strange about the fact that this is what triggers the status change / implements the actual business logic of the queuer, but it does appear to be the convention with other events, so meh


return False

This revision is now accepted and ready to land.Mon, Nov 2, 4:06 PM
johann added inline comments.

Currently they're both the same but eventually I think it's possible queued would want to setup a connection with the heartbeat table or something like that. Then we'd need to do super.initialize. I think I prefer keeping the definition here, but don't have a strong opinion


_instance_weak_ref maybe


Holding off on this because the other methods use SystemExecutionContext which we don't have yet, so we don't have a clean object to pass with all the info. I'll refactor this in the future because stuff like timestamp=time.time() really doesn't belong here


Agreed on meh


Going to save these for the future cancellation diff. There currently are no callsites to it (Dagit's still going to the launcher), which this helps me keep clear

johann marked 2 inline comments as done.


This revision was landed with ongoing or failed builds.Tue, Nov 3, 1:49 AM
This revision was automatically updated to reflect the committed changes.