Skip to content

feat: add airflow triggerer Deployment #521

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.

Sign up for GitHub

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

Jump to bottom
Closed
mmazek wants to merge 1 commit into from
Closed

feat: add airflow triggerer Deployment #521

mmazek wants to merge 1 commit into from

Conversation

@mmazek
Copy link

@mmazek mmazek commented Feb 15, 2022 •
edited by thesuperzapper
Loading

What issues does your PR fix?

What does your PR do?

  • Adds a new Deployment for the airflow triggerer so we fully support Airflow's Deferrable Tasks feature.
    • WARNING: airflow triggerer was added in airflow 2.2.0, so will not work in previous versions

Checklist

For all Pull Requests

For releasing ONLY

@thesuperzapper thesuperzapper changed the title feat: Add support for Airflow Triggerer feat: add airflow triggerer Deployment Feb 17, 2022
thesuperzapper
thesuperzapper requested changes Feb 17, 2022
View reviewed changes
Copy link
Member

@thesuperzapper thesuperzapper left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @mmazek!

I have left some comments inline.

PS: remember to sign off your commits, to keep the DCO bot happy

@mmazek
Copy link
Author

mmazek commented Feb 18, 2022

Hi @thesuperzapper, thank you for the feedback! I fixed all the issues you pointed out and also made the triggerer enabled by default.

thesuperzapper
thesuperzapper requested changes Feb 22, 2022
View reviewed changes
Copy link
Member

@thesuperzapper thesuperzapper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mmazek great work so far, but still a few quick changes are required to get it merged.

thesuperzapper
thesuperzapper requested changes Feb 23, 2022
View reviewed changes
Copy link
Member

@thesuperzapper thesuperzapper left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmazek Upon testing I realized that the --capacity parameter actually does not work (its airflow's fault, see apache/airflow#21752), setting it at all causes the triggerer to crash.

thesuperzapper
thesuperzapper requested changes Feb 25, 2022
View reviewed changes
Copy link
Member

@thesuperzapper thesuperzapper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmazek looks good now in terms of the triggerer itself, but still one strange formatting on the README parameter table.

PS: I would have pushed the small change to your branch myself, but I think because you are using the main branch of your fork, it is not letting me push commits to your fork. Or you might have just not ticked Allow edits and access to secrets by maintainers on the PR sidebar.

@mmazek mmazek requested a review from thesuperzapper March 2, 2022 15:18
thesuperzapper
thesuperzapper approved these changes Mar 2, 2022
View reviewed changes
Copy link
Member

@thesuperzapper thesuperzapper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmazek looks good, thanks for all your work and patience getting this ready!

I will merge when I cut the 8.6.0 release (hopefully soon, my goal is to make 8.6.0 the "prod-ready Airflow 2.2.X" release)

@thesuperzapper thesuperzapper added this to the airflow-8.6.0 milestone Mar 2, 2022
@thesuperzapper thesuperzapper added the status/ready-to-merge status - this will be merged into next release label Mar 22, 2022
@thesuperzapper thesuperzapper changed the title feat: add airflow triggerer Deployment feat: add airflow triggerer Deployment Apr 1, 2022
@thesuperzapper thesuperzapper mentioned this pull request Apr 2, 2022
Merged
6 tasks
@thesuperzapper thesuperzapper added status/duplicate status - this issue or pull request already exists and removed status/ready-to-merge status - this will be merged into next release labels Apr 2, 2022
@thesuperzapper
Copy link
Member

thesuperzapper commented Apr 2, 2022

@mmazek in the interest of releasing 8.6.0 ASAP, I have made a new PR #555.

This PR includes your commit (so you should end up with the "contributor" tag).

I had to do this because you did not allow maintainers of the upstream repo to add commits when you created your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thesuperzapper thesuperzapper thesuperzapper approved these changes

@gsemet gsemet Awaiting requested review from gsemet

Assignees

No one assigned

Labels

status/duplicate status - this issue or pull request already exists

Projects

Issue Triage and PR Tracking
Archived in project

Milestone

airflow-8.6.0

Development

Successfully merging this pull request may close these issues.

add airflow triggerer Deployment (support Deferrable Operators)

2 participants

@mmazek @thesuperzapper