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(timetable): CronMixin issue when DST #35632

Closed
wants to merge 1 commit into from

Conversation

V0lantis
Copy link
Contributor

@V0lantis V0lantis commented Nov 14, 2023

Follow-up of #35558.

This is an attempt to figure out how the CronMixin class was working, and especially _get_prev and _get_next methods.

By making timezone aware the scheduled variable in the two methods, I managed to suppress the issue. Therefore, I am questioning the two methods logic (which I still didn't figure out). I found the introduction of those two methods in #17414 (Hello @uranusjr 👋 ). Do we really need to compute the delta with scheduled and naive variables? This is error prone since, during a DST, the delta will not be coherent? If I am missing something, I think we would really benefit from documentation + tests on those two methods.

###$ Before my changes:

the 28 at 20: pre_date=2023-10-28T19:00:00+00:00
the 28 at 21: pre_date=2023-10-28T19:00:00+00:00
the 28 at 22: pre_date=2023-10-28T19:00:00+00:00
the 28 at 23: pre_date=2023-10-28T19:00:00+00:00
the 29 at 0: pre_date=2023-10-28T19:00:00+00:00
the 29 at 1: pre_date=2023-10-28T20:00:00+00:00 <== here it switches
the 29 at 2: pre_date=2023-10-28T20:00:00+00:00
the 29 at 3: pre_date=2023-10-28T20:00:00+00:00
the 29 at 4: pre_date=2023-10-28T20:00:00+00:00
the 29 at 5: pre_date=2023-10-29T05:00:00+00:00
the 29 at 6: pre_date=2023-10-28T20:00:00+00:00 <== Here it is messed up
the 29 at 7: pre_date=2023-10-29T07:00:00+00:00

After them

the 28 at 20: pre_date=2023-10-28T19:00:00+00:00
the 28 at 21: pre_date=2023-10-28T19:00:00+00:00
the 28 at 22: pre_date=2023-10-28T19:00:00+00:00
the 28 at 23: pre_date=2023-10-28T19:00:00+00:00
the 29 at 0: pre_date=2023-10-28T19:00:00+00:00
the 29 at 1: pre_date=2023-10-28T19:00:00+00:00
the 29 at 2: pre_date=2023-10-28T19:00:00+00:00
the 29 at 3: pre_date=2023-10-28T19:00:00+00:00
the 29 at 4: pre_date=2023-10-28T19:00:00+00:00
the 29 at 5: pre_date=2023-10-28T19:00:00+00:00
the 29 at 6: pre_date=2023-10-29T06:00:00+00:00
the 29 at 7: pre_date=2023-10-29T07:00:00+00:00

We notice that the the value are coherent in the second batch


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

@V0lantis V0lantis requested a review from uranusjr as a code owner November 14, 2023 17:33
@V0lantis V0lantis marked this pull request as draft November 14, 2023 17:33
@V0lantis V0lantis force-pushed the fix/align-to-prev-cron-mixin branch from 28ebb5e to cf0fc2f Compare November 14, 2023 17:37
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 Dec 30, 2023
@github-actions github-actions bot closed this Jan 4, 2024
@uranusjr
Copy link
Member

uranusjr commented Jan 4, 2024

Is this still relevant after #35887? A rebase is needed. Also I’d change the test to use an actual timetable instead of testing the mixin directly, since that’s what we actually care about.

@V0lantis
Copy link
Contributor Author

V0lantis commented Jan 4, 2024

Thank you @uranusjr, will try to look into this soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale PRs per the .github/workflows/stale.yml policy file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants