select unassigned asset triggers from db#65792
Conversation
I think @ashb or @uranusjr are the biggest experts on that field. Looks good to me but a proof of logging that it works would maybe be an option. Also dunno how SQLAlchemy queries are being unit tested at all. Would be good if we would have a unit test that guards this specific code from regression if future changes would occur. WDYT @jscheffl ? |
Would be good having a unit test but testing the query for elements is a bit self-fulfuilling. Not sure how a "good test" would look like. I have only bad tests in mind which do not help. But I see there are explicit 4 tests. @renat-sagut can you add a test for this (Or extend one of the existing tests for this)? |
|
Agreed, generally the important thing is to test the behaviour of the query, not the exact query produced |
ashb
left a comment
There was a problem hiding this comment.
This query uses SELECT ... FOR UPDATE SKIP LOCKED so only one Triggerer will get each row.
What about unit tests?
added tests |
cd1ffa3 to
52da60e
Compare
potiuk
left a comment
There was a problem hiding this comment.
LGTM — small, well-targeted fix that brings the asset-trigger query in line with the task-instance-trigger query at line 427 (alive_triggerer_ids filter). Test coverage is solid: unassigned-included, dead-triggerer-included, alive-triggerer-excluded, and ordering+capacity. Following @jscheffl's approval.
(Out of scope for this PR, but worth a quick look as a follow-up: the callback-trigger branch a few lines above doesn't carry the same alive_triggerer_ids filter — if callbacks can race in the same way, that may want the same fix.)
This review was drafted by an AI-assisted tool and confirmed by an Apache Airflow maintainer. The maintainer approving this PR has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.
More on how Apache Airflow handles maintainer review: contributing-docs/05_pull_requests.rst.
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
|
Marking for backporting to have this in next patch release. As no other feedback... merging now... |
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
Backport successfully created: v3-2-testNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
|
* select unassigned triggers from db * tests * cleanup (cherry picked from commit 8546c91) Co-authored-by: renat-sagut <renat.sagut@gmail.com>
* select unassigned triggers from db * tests * cleanup (cherry picked from commit 8546c91) Co-authored-by: renat-sagut <renat.sagut@gmail.com>
Fixes a bug where multiple triggerer replicas could pick up the same asset trigger from db. The query now selects only unassigned triggers, similar to task instance triggers.
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.