Fix assignment of unassigned triggers#21770
Conversation
Previously, the query returned no alive triggerers which resulted in all triggers to be assigned to the current triggerer. This works fine, despite the logic bug, in the case where there's a single triggerer. But with multiple triggerers, concurrent iterations of the TriggerJob loop would bounce trigger ownership to whichever loop ran last. Addresses #21616
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
dstandish
left a comment
dstandish
left a comment
There was a problem hiding this comment.
looks like a great fix, and nice work with the test
| trigger_ids_query = ( | ||
| session.query(cls.id).filter(cls.triggerer_id.notin_(alive_triggerer_ids)).limit(capacity).all() | ||
| session.query(cls.id) | ||
| .filter(or_(cls.triggerer_id.notin_(alive_triggerer_ids), cls.triggerer_id == None)) # noqa: E711 |
There was a problem hiding this comment.
if the triggerer_id == None, then wouldn't it already be true that it's not in alive_triggerer_ids?
There was a problem hiding this comment.
no, unfortunately that's a quirk with sqlalchemy's notin_ (test fails when that's removed). I'll add a comment to that effect
There was a problem hiding this comment.
ok makes sense.
small thing but can i also suggest in that case, put the cls.triggerer_id.is_(None) first in the OR
then it reads more like "not assigned OR on a dead triggerer"
compared with "on a dead triggerer OR not assigned"
"not assigned" is by far the more common / standard / expected case i think so seems like it would be more intuitive to put it first. and, it sort of avoids that issue where first condition looks like it should cover second.
There was a problem hiding this comment.
make sense to me
| session.commit() | ||
| assert session.query(Trigger).count() == 3 | ||
| Trigger.assign_unassigned(new_triggerer.id, 100, session=session) | ||
| session.expire_all() |
There was a problem hiding this comment.
why not commit instead? genuine question -- still learning sqlalchemy here. but from the doc it indicates that when you commit, then everything gets expired anyway. and committing seems like a more intuitive thing to do here.
There was a problem hiding this comment.
had a bunch of trouble here (and I suspect it's because of the synchronize_session=False in Trigger.assign_unassigned). at some point I did try various combinations of session.flush(), session.commit(), but this is what worked in getting the state synced. I'm no sqlalchemy expert either!
dstandish
left a comment
dstandish
left a comment
There was a problem hiding this comment.
LGTM, i may try it locally before merging, and it's core so should have second set of eyes.
github-actions
Bot
added
the
full tests needed
|
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
|
Cool thanks @dstandish |
|
LGTM as well . |
|
Feel free to merge @dstandish |
|
Awesome work, congrats on your first merged pull request! |
Previously, the query returned no alive triggerers which resulted in all triggers to be assigned to the current triggerer. This works fine, despite the logic bug, in the case where there's a single triggerer. But with multiple triggerers, concurrent iterations of the TriggerJob loop would bounce trigger ownership to whichever loop ran last. Addresses #21616 (cherry picked from commit b26d4d8)
Previously, the query returned no alive triggerers which resulted in all triggers to be assigned to the current triggerer. This works fine, despite the logic bug, in the case where there's a single triggerer. But with multiple triggerers, concurrent iterations of the TriggerJob loop would bounce trigger ownership to whichever loop ran last. Addresses #21616 (cherry picked from commit b26d4d8)
Previously, the query returned no alive triggerers which resulted in all triggers to be assigned to the current triggerer. This works fine, despite the logic bug, in the case where there's a single triggerer. But with multiple triggerers, concurrent iterations of the TriggerJob loop would bounce trigger ownership to whichever loop ran last. Addresses #21616 (cherry picked from commit b26d4d8)
Previously, the query returned no alive triggerers which resulted in all triggers to be assigned to the current triggerer. This works fine, despite the logic bug, in the case where there's a single triggerer. But with multiple triggerers, concurrent iterations of the TriggerJob loop would bounce trigger ownership to whichever loop ran last. Addresses #21616 (cherry picked from commit b26d4d8)
Previously, the query returned no alive triggerers which resulted in all triggers to be assigned to the current triggerer. This works fine, despite the logic bug, in the case where there's a single triggerer. But with multiple triggerers, concurrent iterations of the TriggerJob loop would bounce trigger ownership to whichever loop ran last. Addresses #21616 (cherry picked from commit b26d4d8)
Previously, the query returned no alive triggerers which resulted in all triggers to be assigned to the current triggerer. This works fine, despite the logic bug, in the case where there's a single triggerer. But with multiple triggerers, concurrent iterations of the TriggerJob loop would bounce trigger ownership to whichever loop ran last. Addresses #21616 (cherry picked from commit b26d4d8)
Previously, the query returned no alive triggerers which resulted in all triggers to be assigned to the current triggerer. This works fine, despite the logic bug, in the case where there's a single triggerer. But with multiple triggerers, concurrent iterations of the TriggerJob loop would bounce trigger ownership to whichever loop ran last. Addresses #21616 (cherry picked from commit b26d4d8)
Previously, the query returned no alive triggerers which resulted in all triggers to be assigned to the current triggerer. This works fine, despite the logic bug, in the case where there's a single triggerer. But with multiple triggerers, concurrent iterations of the TriggerJob loop would bounce trigger ownership to whichever loop ran last. Addresses #21616 (cherry picked from commit b26d4d8)
4 participants
Previously, the query returned no alive triggerers which resulted
in all triggers to be assigned to the current triggerer. This works
fine, despite the logic bug, in the case where there's a single
triggerer. But with multiple triggerers, concurrent iterations of
the TriggerJob loop would bounce trigger ownership to whichever
loop ran last.
Addresses #21616
Closes: #21616