Adds the interface to the external runs coordinator
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)
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
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
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