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

Fix _get_count in sensor_helper.py #40795

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fuatcakici
Copy link
Contributor

@fuatcakici fuatcakici commented Jul 15, 2024

Fixes an overlooked part of #39616

Currently, the _get_count function in sensor_helper.py does not return the number of the Dag Runs where all the sub tasks in a task group for all the datetimes given in the dttm_filter have succeeded, but instead just returns 1 if they are all complete. However, this is not the expected behaviour wherever this function is used (namely in the run function in WorkflowTrigger and poke function in ExternalTaskSensor)


^ 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.

@fuatcakici fuatcakici force-pushed the fix-_get_count-in-sensor_helper.py branch 3 times, most recently from d5aab13 to df159e0 Compare July 16, 2024 07:15
@eladkal eladkal added this to the Airflow 2.10.0 milestone Jul 16, 2024
@eladkal eladkal added the type:bug-fix Changelog: Bug Fixes label Jul 16, 2024
@eladkal eladkal force-pushed the fix-_get_count-in-sensor_helper.py branch from df159e0 to 8ee2f7e Compare July 22, 2024 20:38
Copy link

github-actions bot commented Oct 1, 2024

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 Oct 1, 2024
@eladkal
Copy link
Contributor

eladkal commented Oct 1, 2024

@pankajastro @nathadfield Can you check if this fix still needed?

@github-actions github-actions bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Oct 2, 2024
@nathadfield
Copy link
Collaborator

@eladkal Well, I fixed the original issue. It seems this is related to something I overlooked but I'm not aware that it's a problem.

@fuatcakici
Copy link
Contributor Author

fuatcakici commented Oct 3, 2024

@nathadfield yes, I was also waiting for that issue to be fixed, so thanks! But basically here, there are no issues if there is only one execution date set for the execution_dates parameter in the WorkflowTrigger. But once you set more than one execution date, it will just hang since _get_count will give 1 once all the tasks finish, but WorkflowTrigger will wait until it gives len(execution_dates), which will not happen.

@nathadfield
Copy link
Collaborator

@fuatcakici Ok, well, this is your PR but it originally had some issues. Can I suggest that you rebase it and then this should trigger the tests again?

@fuatcakici fuatcakici force-pushed the fix-_get_count-in-sensor_helper.py branch from 8ee2f7e to 46c0fa8 Compare October 7, 2024 09:37
@fuatcakici
Copy link
Contributor Author

@nathadfield Rebased 🙂

@nathadfield
Copy link
Collaborator

@fuatcakici Great! It looks like all the tests passed too although I don't have the ability to merge. Maybe @eladkal or @pankajastro might be able to oblige?

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 26, 2024
@eladkal
Copy link
Contributor

eladkal commented Nov 26, 2024

@fuatcakici can you rebase and resolve conflicts?
Also we need unit test to avoid regression

@fuatcakici fuatcakici force-pushed the fix-_get_count-in-sensor_helper.py branch from 46c0fa8 to 39e5061 Compare November 26, 2024 09:41
Fixes an overlooked part of apache#39616

Currently, the _get_count function in sensor_helper.py does not return the number of the Dag Runs where all the sub tasks in a task group for all the datetimes given in the dttm_filter have succeeded, but instead just returns 1 if they are all complete. However, this is not the expected behaviour wherever this function is used (namely in the run function in WorkflowTrigger and poke function in ExternalTaskSensor)
@fuatcakici fuatcakici force-pushed the fix-_get_count-in-sensor_helper.py branch from 39e5061 to 83b1eb6 Compare November 26, 2024 10:26
@fuatcakici
Copy link
Contributor Author

@eladkal Rebased once again and all the tests succeeded 🙂 There are of course many changes happening in real time so it is virtually impossible for the branch to stay constantly up-to-date

@eladkal eladkal removed this from the Airflow 2.11.0 milestone Nov 26, 2024
@eladkal
Copy link
Contributor

eladkal commented Nov 26, 2024

Since @nathadfield confirmed it's a bug fix, I am happy to merge once you add a unit test to cover this change. This is needed to making sure we won't have future regression

@eladkal eladkal removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 26, 2024
@eladkal eladkal added area:providers provider:standard and removed type:bug-fix Changelog: Bug Fixes labels Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants