-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
Add optional data interval to CronTriggerTimetable #25503
Add optional data interval to CronTriggerTimetable #25503
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the heads-up. The idea looks good to me. Let me share my feedback.
a59f47e
to
ea15e3a
Compare
This allows the user to provide a (static) data interval for DAG runs to adopt. This should be useful for most cron usages and more natural to the old schedule_interval logic. Also moved CronTriggerTimetable to its own module. The class living in 'interval' is weird.
ea15e3a
to
4b013e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! By the way, I'm not offending you but curious why you always force-push your updates? It makes me a little bit harder to find what you update.
I prefer to do fixup commits for fixes that are not standalone to keep the history clean. I admit GitHub does not provide a very good UI for them, but the changes are still visible if you click on “force-pushed” in the message. |
Oh, I missed |
This allows the user to provide a (static) data interval for DAG runs to adopt. This should be useful for most cron usages and more natural to the old schedule_interval logic. If
interval
is not provided, the default behaviour is the same as implemented in #23662.Also moved CronTriggerTimetable to its own module. The class living in
interval
is weird.cc @mai-nakagawa