You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "avkirilishin (via GitHub)" <gi...@apache.org> on 2023/12/09 00:14:57 UTC
[PR] EventsTimetable schedules past events even if catchup=False [airflow]
avkirilishin opened a new pull request, #36134:
URL: https://github.com/apache/airflow/pull/36134
closes: https://github.com/apache/airflow/issues/33948
Fixed the bug and added some more tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Do not let EventsTimetable schedule past events if catchup=False [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #36134:
URL: https://github.com/apache/airflow/pull/36134#issuecomment-1853648443
Looks right to me in general, just a minor idea on implementation. I also pushed a commit to improve typing slightly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Do not let EventsTimetable schedule past events if catchup=False [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #36134:
URL: https://github.com/apache/airflow/pull/36134#discussion_r1425150434
##########
airflow/timetables/events.py:
##########
@@ -79,22 +83,34 @@ def next_dagrun_info(
last_automated_data_interval: DataInterval | None,
restriction: TimeRestriction,
) -> DagRunInfo | None:
- if last_automated_data_interval is None:
- next_event = self.event_dates[0]
- else:
- future_dates = itertools.dropwhile(
- lambda when: when <= last_automated_data_interval.end, # type: ignore
- self.event_dates,
- )
- next_event = next(future_dates, None) # type: ignore
- if next_event is None:
- return None
+ earliest = restriction.earliest
+ if not restriction.catchup:
+ current_time = timezone.utcnow()
+ if earliest is None or current_time > earliest:
+ earliest = pendulum.instance(current_time)
+
+ dates = iter(self.event_dates)
+ next_event = next(dates, None)
+ while next_event:
+ is_allowed = True
+ if earliest and next_event < earliest:
+ is_allowed = False
+ if last_automated_data_interval and next_event <= last_automated_data_interval.end:
+ is_allowed = False
+ if is_allowed:
+ break
+ next_event = next(dates, None)
+ if next_event is None:
+ return None
Review Comment:
```suggestion
if not self.event_dates:
return None
for next_event in self.event_dates:
if earliest and next_event < earliest:
continue
elif last_automated_data_interval and next_event <= last_automated_data_interval.end:
continue
break
```
Not exactly sure but this seems to be logically equivalent?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Do not let EventsTimetable schedule past events if catchup=False [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal merged PR #36134:
URL: https://github.com/apache/airflow/pull/36134
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] EventsTimetable schedules past events even if catchup=False [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #36134:
URL: https://github.com/apache/airflow/pull/36134#discussion_r1421437692
##########
airflow/timetables/events.py:
##########
@@ -79,22 +82,34 @@ def next_dagrun_info(
last_automated_data_interval: DataInterval | None,
restriction: TimeRestriction,
) -> DagRunInfo | None:
- if last_automated_data_interval is None:
- next_event = self.event_dates[0]
- else:
- future_dates = itertools.dropwhile(
- lambda when: when <= last_automated_data_interval.end, # type: ignore
- self.event_dates,
- )
- next_event = next(future_dates, None) # type: ignore
- if next_event is None:
- return None
+ earliest = restriction.earliest
+ if not restriction.catchup:
+ current_time = pendulum.DateTime.utcnow()
Review Comment:
I would recommend to use `airflow.utils.timezone.utcnow` for this one instead of direct use `pendulum.DateTime.utcnow()`.
We might potentially get rid of `pendulum` in the future, and this might help to replace this easier
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] EventsTimetable schedules past events even if catchup=False [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on PR #36134:
URL: https://github.com/apache/airflow/pull/36134#issuecomment-1851018689
@uranusjr As you are tagged as Codeowner, can you make another review before merge? Alternatively @Taragolis please make a second pass and then OK to merge?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org