You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/12/22 09:07:56 UTC
[GitHub] [airflow] uranusjr opened a new pull request, #28532: Consider previous run in CronTriggerTimetable
uranusjr opened a new pull request, #28532:
URL: https://github.com/apache/airflow/pull/28532
Previously, when catchup is False, the timetable always calculate the next run against utcnow(), which is incorrect. This new implementation takes the previous run info into account and promises the next run will be after that.
See threadd in https://github.com/apache/airflow/pull/28411#discussion_r1053453973
--
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
[GitHub] [airflow] uranusjr commented on pull request #28532: Consider previous run in CronTriggerTimetable
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #28532:
URL: https://github.com/apache/airflow/pull/28532#issuecomment-1362730727
Yeah the problem with `id` is it shows up in pytest’s test function output so it’s can be too long and should preferrably look like… well an ID. Would have made life easier if there’s a `description` or something available.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #28532: Consider previous run in CronTriggerTimetable
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #28532:
URL: https://github.com/apache/airflow/pull/28532#discussion_r1055271971
##########
airflow/timetables/trigger.py:
##########
@@ -82,18 +82,19 @@ def next_dagrun_info(
restriction: TimeRestriction,
) -> DagRunInfo | None:
if restriction.catchup:
- if last_automated_data_interval is None:
- if restriction.earliest is None:
- return None
- next_start_time = self._align_to_next(restriction.earliest)
- else:
+ if last_automated_data_interval is not None:
next_start_time = self._get_next(last_automated_data_interval.end)
- else:
- current_time = DateTime.utcnow()
- if restriction.earliest is not None and current_time < restriction.earliest:
- next_start_time = self._align_to_next(restriction.earliest)
+ elif restriction.earliest is None:
+ return None # Don't know where to catch up from, give up.
Review Comment:
NIT. Could we add a test case for that one? I could not find it - and I think we should have this edge case in the tests to avoid regression (and also maybe some explanation what case it describes ?|)
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #28532: Consider previous run in CronTriggerTimetable
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28532:
URL: https://github.com/apache/airflow/pull/28532#discussion_r1057096371
##########
airflow/timetables/trigger.py:
##########
@@ -82,18 +82,19 @@ def next_dagrun_info(
restriction: TimeRestriction,
) -> DagRunInfo | None:
if restriction.catchup:
- if last_automated_data_interval is None:
- if restriction.earliest is None:
- return None
- next_start_time = self._align_to_next(restriction.earliest)
- else:
+ if last_automated_data_interval is not None:
next_start_time = self._get_next(last_automated_data_interval.end)
- else:
- current_time = DateTime.utcnow()
- if restriction.earliest is not None and current_time < restriction.earliest:
- next_start_time = self._align_to_next(restriction.earliest)
+ elif restriction.earliest is None:
+ return None # Don't know where to catch up from, give up.
Review Comment:
Added
--
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
[GitHub] [airflow] uranusjr merged pull request #28532: Consider previous run in CronTriggerTimetable
Posted by GitBox <gi...@apache.org>.
uranusjr merged PR #28532:
URL: https://github.com/apache/airflow/pull/28532
--
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