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

Make it possible to change the default cron timetable #34851

Merged
merged 11 commits into from
Jan 4, 2024

Conversation

collinmcnulty
Copy link
Contributor

@collinmcnulty collinmcnulty commented Oct 10, 2023

Closes #34695 .

Adds the setting scheduler.legacy_cron_data_intervals with a default of True (CronDataIntervalTimetable). If it is set to False, the CronTriggerTimetable is used for any bare cron strings.


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

@uranusjr

This comment was marked as resolved.

@uranusjr
Copy link
Member

I feel this is a good idea. It may be worthwhile to expand this a bit to be a full “policy” that handles all kinds of non-timetable/dataset inputs, but that could be expanded later, with this maybe retained for convenience or compatibility.

@collinmcnulty
Copy link
Contributor Author

Yes, linked issue was wrong, I have now fixed it.

@collinmcnulty
Copy link
Contributor Author

I am not sure how best to unit test this.

@uranusjr
Copy link
Member

Because conf is not called conf in this file

from airflow.configuration import conf as airflow_conf, secrets_backend_list

@collinmcnulty collinmcnulty marked this pull request as ready for review October 31, 2023 17:56
airflow/config_templates/config.yml Outdated Show resolved Hide resolved
airflow/models/dag.py Outdated Show resolved Hide resolved
airflow/models/dag.py Outdated Show resolved Hide resolved
@collinmcnulty
Copy link
Contributor Author

@uranusjr I've responded to all review comments.

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 24, 2023
@uranusjr uranusjr removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 25, 2023
@collinmcnulty
Copy link
Contributor Author

I'm pretty sure the failing test is spurious. I do not have permission to re-run it.

@potiuk
Copy link
Member

potiuk commented Dec 30, 2023

I'm pretty sure the failing test is spurious. I do not have permission to re-run it.

Neither do we once there is conflict with another PR. You need to rebase and solve the conflicts in order to re-run the tests.

@potiuk
Copy link
Member

potiuk commented Dec 30, 2023

Screenshot 2023-12-30 at 07 15 08

@potiuk
Copy link
Member

potiuk commented Jan 3, 2024

This is fine, however I think we need to add a bit more education for our users on it. We have about a million configuration parameters and expecting the users to read them and understand them in order to know they can change such a basic behavioud of Airlfow is s mostly wishful thinking.

I think this deserve some more important and prominent explanation soemwhere close to "Concepts" - possibly at the "Concept" page itself.

I think we should have there a chapter entitled:

"How Airflow Schedule Cron-like schedules" - where we should explain how it was, originally and how users can change it to get to more "familiar" scheduling semantics. This is a bit too important change to mention it in only in configuration parameter description IMHO

@collinmcnulty
Copy link
Contributor Author

I fully agree. Would you be ok with that being a separate PR?

@potiuk
Copy link
Member

potiuk commented Jan 4, 2024

I fully agree. Would you be ok with that being a separate PR?

Sure. That's why I did not request changes :)

@potiuk potiuk merged commit ac4073b into apache:main Jan 4, 2024
51 checks passed
@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Jan 10, 2024
@ephraimbuddy ephraimbuddy added this to the Airflow 2.9.0 milestone Jan 10, 2024
abhishekbhakat pushed a commit to abhishekbhakat/my_airflow that referenced this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuration setting to make CronTriggerTimetable default
5 participants