Page MenuHomePhabricator

Run queue docs
ClosedPublic

Authored by johann on Fri, Jan 8, 3:32 PM.

Details

Summary

Missing links to be filled in.

Thoughts on the small deployment 'setup' section here? Probably redundant with the other deployment pages

Test Plan

View locally

Diff Detail

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

Event Timeline

johann requested review of this revision.Fri, Jan 8, 3:51 PM
johann retitled this revision from Run concurrency docs to [Draft] Run concurrency docs.Fri, Jan 8, 4:04 PM
johann edited the summary of this revision. (Show Details)
johann added reviewers: dgibson, prha.
johann added a reviewer: yuhan.
dgibson requested changes to this revision.Fri, Jan 8, 9:54 PM

Generally looks good, few rambling thoughts.

as discussed offline i think since this is primarily instance config it should go on the Overviews (maybe alongside run launcher and executors, and rename that section to Runs or something? Not perfect. You could also call that section Run Coordinator (or Queueing Runs maybe) - somewhere in here we should probably explain succinctly what a Run Coordinator represents and how it relates to a Run Launcher since that could be confusing)

docs/next/src/pages/deploying/limiting_run_concurrency.mdx
18 ↗(On Diff #29066)

maybe contrast with the default run coordinator here, and explain what a run coordinator is? maybe make it explicit that with this coordinator, dagit just puts the runs in a queue, and its the daemon that does the actual queue management and launch

You could also relate this to the run launcher - before a run goes into the run launcher, it first gets submitted to the run coordinator.

20 ↗(On Diff #29066)

link to the instance overview?

31 ↗(On Diff #29066)

this might be out of scope for the doc if this is moved out of the deploy section

59 ↗(On Diff #29066)

You could remove 'The limits are restrictive', i think it's clear without.

62 ↗(On Diff #29066)

maybe start with the default behavior and then contrast? "By default, runs will be removed from the queue in the order that they are added"

64–65 ↗(On Diff #29066)

I see the goal here (explaining that it's not just a single fixed queue) but I found the explanation here somewhat hard to follow. It's kind of the flip side of the sentence above about 'if any limit would be exceeded by launching a run, then the run will stay queued.' right? This could go alongside that. "If no limit would be exceeded, then the run with the highest priorit will launch".

And maybe make it explicit that if multiple runs are eligible to launch and have the same priority, the first one in will launch first? (Assuming that's true, I'm not sure it is)

This revision now requires changes to proceed.Fri, Jan 8, 9:54 PM

feedback

docs/next/src/pages/deploying/limiting_run_concurrency.mdx
64–65 ↗(On Diff #29066)

Agreed it's a bit hard to follow.

I think the critical bit is However, a run that is blocked by tag limits will not block runs submitted after it, maybe the example is overkill?

johann retitled this revision from [Draft] Run concurrency docs to Run queue docs.Wed, Jan 13, 7:18 PM
dgibson added inline comments.
docs/next/src/pages/overview/pipeline-runs/limiting-run-concurrency.mdx
39–42

Maybe worth walking through the example a bit after stating it.

"This run queue will only allow a maximum of 25 runs at once. Additionally, only 4 runs with the "database" tag equal to "redshift" can run at once, and at most 10 runs with the "dagster/backfill" key set to any value can run at once".

This revision is now accepted and ready to land.Wed, Jan 13, 9:34 PM
This revision was landed with ongoing or failed builds.Wed, Jan 13, 10:56 PM
Closed by commit R1:09da21d38ae2: Run queue docs (authored by johann). · Explain Why
This revision was automatically updated to reflect the committed changes.