Fix lost runs in CronTriggerTimetable
#32921
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
BohdanSemonov
changed the title
BohdanSemonov
changed the title
BohdanSemonov
changed the title
| @@ -89,7 +89,7 @@ def next_dagrun_info( | |||
| else: | |||
| next_start_time = self._align_to_next(restriction.earliest) | |||
| else: | |||
| start_time_candidates = [self._align_to_next(DateTime.utcnow())] | |||
| start_time_candidates = [self._align_to_next(DateTime.utcnow().replace(second=0, microsecond=0))] | |||
There was a problem hiding this comment.
This makes sense 🤔
Could you add a test for this change? (a test which fails without resetting seconds and microsecond and passes after applying your change)
There was a problem hiding this comment.
Nice one, yeah. That would definitely explain occasionally lost dag runs
There was a problem hiding this comment.
And yeah - having a test for that would be fantastic - if you need help @BohdanSemonov - we can suggest an approach here.
There was a problem hiding this comment.
Wouldn’t this still have problems if the scheduler is a tad too slow and fails to schedule the DAG in a minute? I still prefer the _align_to_prev fix over this.
There was a problem hiding this comment.
@uranusjr - could you please elaborate? Is there a fix in progress you are referring to? Or some new code to be written? It wasn't too clear. I think it would be worth to consider fixing this problem for 2.7.0
There was a problem hiding this comment.
The linked issue already contains discussion before this fix was proposed.
There was a problem hiding this comment.
Yeah. There are 3 linked issues and each of them has tens of comments. Maybe that is obvious to you, but would be great to point to it. A link to specific comment would be great, otherwise it forces somoene looking here to read the entire conversation (and few of those) to find it..
There was a problem hiding this comment.
(Just try to see the other side of people who are participating in multiple discussions and might not remember all of it).
There was a problem hiding this comment.
I believe he means the change shown in these 2 comments. Apologies for interjecting, but I am excited for this fix so wanted to help keep things moving.
There was a problem hiding this comment.
I think it relies on @uranusjr -> I am unable to assess if it's good enough or not at this stage and how to move forward.
hussein-awala
changed the title
CronTriggerTimetable by resetting seconds and microseconds in start_time_candidates
hussein-awala
changed the title
CronTriggerTimetable by resetting seconds and microseconds in start_time_candidatesCronTriggerTimetable
closes: #27399
Fix lost runs in
CronTriggerTimetableby resetting seconds and microseconds instart_time_candidates.^ 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.rstor{issue_number}.significant.rst, in newsfragments.