Page MenuHomePhabricator

run queueing: Support disabling max_concurrent_runs
AbandonedPublic

Authored by johann on Nov 4 2020, 5:20 PM.

Details

Summary

Currently you can't use the queued run coordinator without setting a max runs limit.

Test Plan

Since this is the new default, both disabled and enabled case are covered by existing tests

Diff Detail

Repository
R1 dagster
Branch
queuer-disable-max-runs (branched from master)
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

johann edited the summary of this revision. (Show Details)
johann requested review of this revision.Nov 4 2020, 5:38 PM
dgibson requested changes to this revision.Nov 4 2020, 7:14 PM

I think I'm missing some context here? I'm just thinking about the 'pit of success' here - it seems like the easiest behavior to get yourself into here if you don't bother to read the description and start the daemon up is a dequeue daemon that doesn't actually do any queueing, which seems unlikely to be what most people would want. Could we either make the CLI argument required (and > 0) or set it to a reasonable default?

This revision now requires changes to proceed.Nov 4 2020, 7:14 PM

I'm not sure about making it required, since in places like Helm we'll probably end up providing some default again. Is there a reasonable default that you have in mind? Not opposed to just leaving it at 10.

I think this kind of thing is also one reason I like the idea of putting config like this on the instance rather than in CLI params? as in https://dagster.phacility.com/D5009 - then you don't need to worry about what the limit is if you didn't set up this coordinator, and when you do set that coordinator you can also specify the limit that's specific to that daemon