Skip to content
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

[AIP-86] Add Deadline Alerts table, model, and supporting tests. #44712

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

ferruzzi
Copy link
Contributor

@ferruzzi ferruzzi commented Dec 6, 2024

First step on implementingAIP-86 is to add a new db table.

Adds the new table and related migrations, models, unit tests, etc.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

airflow/models/deadlines.py Outdated Show resolved Hide resolved
airflow/models/deadlines.py Outdated Show resolved Hide resolved
airflow/models/deadlines.py Outdated Show resolved Hide resolved
airflow/models/deadline.py Outdated Show resolved Hide resolved
@ferruzzi
Copy link
Contributor Author

ferruzzi commented Dec 6, 2024

I think maybe this needs the Airflow3: Breaking label to pass a couple of those tests? I'm not entirely sure though, so I'll hold off on that for now. If anyone knows for sure, please feel free to add it.

Many of the failing tests seem to revolve around the new deadline table I'm adding here not existing. I'll have to look at the contributing docs and see if I can figure out what I missed.

@ferruzzi ferruzzi added the airflow3.0:breaking Candidates for Airflow 3.0 that contain breaking changes label Dec 10, 2024
@ferruzzi
Copy link
Contributor Author

I've implemented the suggested change to the table schema.

I had assumed the auto-generated sha file would meet static check requirements, silly me. I'll add a commit fixing that and adding the newsfragment, and I have added the 3.0-breaking label. Let's see if that gets me green.

@vincbeck
Copy link
Contributor

I've implemented the suggested change to the table schema.

I had assumed the auto-generated sha file would meet static check requirements, silly me. I'll add a commit fixing that and adding the newsfragment, and I have added the 3.0-breaking label. Let's see if that gets me green.

Do we need a newsfragment? As far as I know this is not a breaking change

@ferruzzi
Copy link
Contributor Author

Do we need a newsfragment? As far as I know this is not a breaking change

The CI test was failing because I didn't have one, so I added one. I made it 44712.significant, but maybe it should be 44712.feature?

@vincbeck
Copy link
Contributor

Do we need a newsfragment? As far as I know this is not a breaking change

The CI test was failing because I didn't have one, so I added one. I made it 44712.significant, but maybe it should be 44712.feature?

Interesting. I did not know we had such tests. At least, yes, I'd say .feature is better

@vincbeck
Copy link
Contributor

Why do you need the label "airflow3.0:breaking"?

@ferruzzi ferruzzi removed the airflow3.0:breaking Candidates for Airflow 3.0 that contain breaking changes label Dec 10, 2024
@ferruzzi
Copy link
Contributor Author

Cool, looks like that fixed the migration part. I have to fix static checks and rework the unit tests now to account for those changes. Thanks to Ephraim and Daniel for your help on that.

@ferruzzi
Copy link
Contributor Author

ferruzzi commented Dec 17, 2024

Since we are using DagRun.id (a sequential integer) instead of DagRun.run_id (a string) I think calling it run_id is going to cause confusion. Is there any precedent or any suggestion on how to rename that parameter in the Deadline?

I also can't seem to figure out how to fix that fkey constraint failure. I don't suppose one of you might have a sec to have a look at it for me? (@vincbeck @ephraimbuddy @dstandish) I've added asserts to the test to make sure the dagrun is being created in the db and that the dagun.id is what I expect it to be. I'm not sure what the mismatch is, here.

[EDIT] Is this maybe because the DagRun.id is NOT NULLABLE but Deadline.run_id is NULLABLE? I bet that's it....

[EDIT 2] It wasn't. It was because the dag_run.id is always 1 when ruin in series, but unpredictable when tests are run in parallel. Adjusted the unit test accordingly.

- When running the tests in parallel, the dag_run.id is not necessarily `1` like it is when running in series.  Adjusted the unit tests to account for this.
# Conflicts:
#	docs/apache-airflow/img/airflow_erd.sha256
#	docs/apache-airflow/img/airflow_erd.svg
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants