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.

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

Fix mismatch in generated run_id and logical date of DAG run #18707

Merged
merged 8 commits into from Feb 4, 2022
Merged

Fix mismatch in generated run_id and logical date of DAG run #18707

merged 8 commits into from Feb 4, 2022

Conversation

davidcaron
Copy link
Contributor

When execution_date is None, the generated name for the run_id contains the a generated execution_date in isoformat.
That execution_dateshould be passed to trigger_dag, otherwise we have a mismatch between the execution_date in run_id and the actual execution_date of the dag run.

This didn't cause any issues per se, but I thought it should be fixed for consistency.

@davidcaron
fix mismatch in run_id and actual execution_date
9472568 
When execution_date is None, the generated name for the run_id
contains the execution_date in isoformat.
This fixes a mismatch between the execution_date in run_id
and the actual execution_date of the dag run.
@boring-cyborg boring-cyborg bot added the area:core-operators Operators, Sensors and hooks within Core Airflow label Oct 4, 2021
uranusjr
uranusjr reviewed Oct 4, 2021
View reviewed changes
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

LGTM. We should have a test case preventing future regression. I suspect the current test only covers cases where isinstance(self.execution_date, datetime.datetime) is True and thus never detect the bug.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 19, 2021
@uranusjr
Use parsed execution_date in TriggerDagRunOperator
53b971d 
This also allows us to remove the Mypy override in the operator's
__init__ since we now allow the type to be str in the first place, and
the local variable has the correct type.
@uranusjr
Clean up execution_date type validation
e016976
@uranusjr
Merge branch 'main' into fix-execution-date-mismatch-in-trigger-dag
3a6ead2
@uranusjr
Copy link
Member

I added a couple of checks to existing tests to ensure the run ID is generated consistently. I also expanded the refactoring a little to strighten up the type of self.execution_date.

@uranusjr uranusjr removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 22, 2021
@github-actions
Copy link

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.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Nov 22, 2021
@uranusjr
Add tests to ensure consistent run_id
12e3960 
Two checks are added to the TriggerDagRunOperator to ensure the operator
always generates a run_id (if not explicitly specified) from the same
date as execution_date, for consistency.
@eladkal
Copy link
Contributor

eladkal commented Dec 16, 2021

tests seem to fail :(

@uranusjr uranusjr removed the full tests needed We need to run full set of tests for this PR to merge label Jan 13, 2022
@uranusjr
Copy link
Member

I merged main and removed the full-test-needed label, this should help us see where test failures are from.

@eladkal eladkal added this to the Airflow 2.2.4 milestone Jan 20, 2022
@jedcunningham jedcunningham reopened this Jan 27, 2022
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jan 27, 2022
@github-actions
Copy link

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.

@jedcunningham
Copy link
Member

I just merged main and the mssql failures are also happening in main. Still, I'm going to wait on merging this just to be safe.

@uranusjr
Copy link
Member

uranusjr commented Feb 2, 2022

Hmm, looks like MSSQL has some issues with datetime values added in this PR.

@jedcunningham
Copy link
Member

Looks good now!

@uranusjr uranusjr changed the title Fix mismatch in generated run_id and actual execution_date of dag run Fix mismatch in generated run_id and logical date of DAG run Feb 4, 2022
@uranusjr uranusjr merged commit 1f08d28 into apache:main Feb 4, 2022
@jedcunningham jedcunningham added the type:bug-fix Changelog: Bug Fixes label Feb 8, 2022
jedcunningham pushed a commit that referenced this pull request Feb 8, 2022
@davidcaron @jedcunningham
Fix mismatch in generated run_id and logical date of DAG run (#18707)
f0a8dac 
Co-authored-by: Tzu-ping Chung <tp@astronomer.io>
Co-authored-by: Jed Cunningham <jedcunningham@apache.org>
(cherry picked from commit 1f08d28)
jedcunningham pushed a commit that referenced this pull request Feb 10, 2022
@davidcaron @jedcunningham
Fix mismatch in generated run_id and logical date of DAG run (#18707)
7aae75c 
Co-authored-by: Tzu-ping Chung <tp@astronomer.io>
Co-authored-by: Jed Cunningham <jedcunningham@apache.org>
(cherry picked from commit 1f08d28)
jedcunningham pushed a commit that referenced this pull request Feb 17, 2022
@davidcaron @jedcunningham
Fix mismatch in generated run_id and logical date of DAG run (#18707)
1c23405 
Co-authored-by: Tzu-ping Chung <tp@astronomer.io>
Co-authored-by: Jed Cunningham <jedcunningham@apache.org>
(cherry picked from commit 1f08d28)
@jedcunningham jedcunningham mentioned this pull request Feb 18, 2022
Closed
40 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uranusjr uranusjr uranusjr approved these changes

@jedcunningham jedcunningham jedcunningham approved these changes

Assignees
No one assigned
Labels
area:core-operators Operators, Sensors and hooks within Core Airflow full tests needed We need to run full set of tests for this PR to merge type:bug-fix Changelog: Bug Fixes
Projects
None yet
Milestone
Airflow 2.2.4
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@davidcaron @uranusjr @eladkal @jedcunningham