-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Enable triggerer queues #59239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable triggerer queues #59239
Conversation
7934a36 to
e1f2b18
Compare
jason810496
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the nice feature!
Would it be better that Triggerer only add the SQL filter if user specify --consume-trigger-queues from CLI without adding the new configs?
Not sure if this would be not only less ambiguous for users but also make the SQL unchanged by only applying the trigger_queue.in_ filter when users explicitly pass the --consume-trigger-queues flag. In my opinion, this preserves the feature while minimizing changes.
Thanks for the suggestion, I like that idea and am all in favor of reduced ambiguity. It also seems like what your suggesting will fall more in line with the celery queues controls (i.e. the queue(s) a celery worker consumes from are optionally set by the CLI command). |
…ue column max len
65ae27e to
aab098f
Compare
jscheffl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it is good then
jason810496
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#protm , it's a really nice feature for 3.2!
|
Great job! |
|
Indeed! |
We won't be able to rely on the |
* Add trigger_queues to triggerer * Update config additions, add doc section and unit tests * Fix default consume_trigger_queues split on commas * Add migration script, test updates, and news fragment * Address feedback, bugfixes * core unit test typing fix * Fix trigger callback trigger_queue assignment * Fix mypy issues in google provider triggers * Fix 0015_2_9_0_update_trigger_kwargs_type migration * fix deprecated sqlalchemy language in test * refactor trigger_queue in kwargs * Add examples to doc section, clarify newsfragment * Initial refactor to derive trigger queues from task queues * Move trigger queue assignment to task deferral API calls, update docs + tests * Address mypy failures in test_ecs.py * Remove redundant changes from refactor * Make trigger.queue column max length consistent with taskinstance.queue column max len * Remove no longer required typing-only changes * manual refactors to unit test conflict resolutions
Closes: #33818
Partially addresses / related: #59871
What?
queuevalue.--queuesCLI option to the triggerer, to constrain a triggerer to only consume Triggers from a given list of task queues.triggerer.queues_enabledconfig entry, which when set toTrueenables the trigger queue feature (Falseby default).queuecolumn to thetriggertable, which is set automatically by the task runner whentriggerer.queues_enabledis set to True.Why?
Testing
triggerer.queues_enabled=Falseand once withtriggerer.queues_enabled=Trueto ensure deferrable tasks worked in either setup.breezeandprektesting commandsuv run --group docs build-docs --autobuildto check the new doc section rendering.Note on event-based triggers and async Callbacks
Following from the helpful discussion here, this PR aims to assign a trigger's
queuefrom its deferring task'squeue. As such, this PR does not add any triggerer assignment controls for event-based triggers orTriggererCallback-based triggers. This was done intentionally both to keep this PR's scope reasonable, and because the latter 2 types of triggers do not necessarily have a relation to a singular task / task queue. If it is preferred, I'm happy to create a followup issue for adding queue support in these types of triggers. I've also made sure to make this explicit in the user documentation, and added details in the docs on how to use this feature without disrupting any potential usage of event-based or callback-based triggers.