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/16 18:03:01 UTC
[GitHub] [airflow] bbovenzi opened a new pull request, #28411: Fix calendar view for CronTriggerTimeTable dags
bbovenzi opened a new pull request, #28411:
URL: https://github.com/apache/airflow/pull/28411
We were checking the wrong instance when building the calendar data for CronTrigger dags and then getting stuck in an infinite loop.
Fixes: https://github.com/apache/airflow/issues/27645
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
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] bbovenzi commented on a diff in pull request #28411: Fix calendar view for CronTriggerTimeTable dags
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #28411:
URL: https://github.com/apache/airflow/pull/28411#discussion_r1053517558
##########
airflow/www/views.py:
##########
@@ -2828,7 +2828,7 @@ def _convert_to_date(session, column):
restriction = TimeRestriction(dag.start_date, dag.end_date, False)
dates = collections.Counter()
- if isinstance(dag.timetable, CronDataIntervalTimetable):
+ if isinstance(dag.timetable, CronTriggerTimetable):
Review Comment:
1. Included both cron timetables in the if statement
2. `next_dagrun_info` was always the same [here](https://github.com/apache/airflow/blob/main/airflow/www/views.py#L2844). So we never reached either of the `break` statements.
--
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 #28411: Fix calendar view for CronTriggerTimeTable dags
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28411:
URL: https://github.com/apache/airflow/pull/28411#discussion_r1055207870
##########
airflow/www/views.py:
##########
@@ -2828,7 +2828,7 @@ def _convert_to_date(session, column):
restriction = TimeRestriction(dag.start_date, dag.end_date, False)
dates = collections.Counter()
- if isinstance(dag.timetable, CronDataIntervalTimetable):
+ if isinstance(dag.timetable, CronTriggerTimetable):
Review Comment:
Ah I get it, because CronTriggerTimetable always uses the current time when catchup=False without considering `last_automated_data_interval`. It should; I’ll fix it in a different PR.
--
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 #28411: Fix calendar view for CronTriggerTimeTable dags
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28411:
URL: https://github.com/apache/airflow/pull/28411#discussion_r1055367810
##########
airflow/www/views.py:
##########
@@ -2828,7 +2828,7 @@ def _convert_to_date(session, column):
restriction = TimeRestriction(dag.start_date, dag.end_date, False)
dates = collections.Counter()
- if isinstance(dag.timetable, CronDataIntervalTimetable):
+ if isinstance(dag.timetable, CronTriggerTimetable):
Review Comment:
This would still serve as an optimisation.
--
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] ashb commented on a diff in pull request #28411: Fix calendar view for CronTriggerTimeTable dags
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #28411:
URL: https://github.com/apache/airflow/pull/28411#discussion_r1055351738
##########
airflow/www/views.py:
##########
@@ -2828,7 +2828,7 @@ def _convert_to_date(session, column):
restriction = TimeRestriction(dag.start_date, dag.end_date, False)
dates = collections.Counter()
- if isinstance(dag.timetable, CronDataIntervalTimetable):
+ if isinstance(dag.timetable, CronTriggerTimetable):
Review Comment:
@uranusjr Does your PR mean we don't need this PR at all?
--
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 #28411: Fix calendar view for CronTriggerTimeTable dags
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28411:
URL: https://github.com/apache/airflow/pull/28411#discussion_r1055176590
##########
airflow/www/views.py:
##########
@@ -2828,7 +2828,7 @@ def _convert_to_date(session, column):
restriction = TimeRestriction(dag.start_date, dag.end_date, False)
dates = collections.Counter()
- if isinstance(dag.timetable, CronDataIntervalTimetable):
+ if isinstance(dag.timetable, CronTriggerTimetable):
Review Comment:
Hmm, that does not sound right, the timetable should always be progressing forward. @mai-nakagawa maybe you could spot what’s wrong here?
(In the mean time I’m going to add a check to prevent infinite loop from happening)
--
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 #28411: Fix calendar view for CronTriggerTimeTable dags
Posted by GitBox <gi...@apache.org>.
uranusjr merged PR #28411:
URL: https://github.com/apache/airflow/pull/28411
--
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 #28411: Fix calendar view for CronTriggerTimeTable dags
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28411:
URL: https://github.com/apache/airflow/pull/28411#discussion_r1053453973
##########
airflow/www/views.py:
##########
@@ -2828,7 +2828,7 @@ def _convert_to_date(session, column):
restriction = TimeRestriction(dag.start_date, dag.end_date, False)
dates = collections.Counter()
- if isinstance(dag.timetable, CronDataIntervalTimetable):
+ if isinstance(dag.timetable, CronTriggerTimetable):
Review Comment:
Hmm, actually I think this was intended for CronDataIntervalTimetable, since the code was initially an optimisation to make rendering faster, and actually predates CronTriggerTimetable.
So I think we should do two things
1. _Add_ CronTriggerTimetable to the check (not replace)
2. Figure out why CronTriggerTimetable does not work with the `else` branch and fix that.
--
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 #28411: Fix calendar view for CronTriggerTimeTable dags
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28411:
URL: https://github.com/apache/airflow/pull/28411#discussion_r1053456841
##########
airflow/www/views.py:
##########
@@ -2828,7 +2828,7 @@ def _convert_to_date(session, column):
restriction = TimeRestriction(dag.start_date, dag.end_date, False)
dates = collections.Counter()
- if isinstance(dag.timetable, CronDataIntervalTimetable):
+ if isinstance(dag.timetable, CronTriggerTimetable):
Review Comment:
This logic was added in #24262; CronTriggerTimetable was added in #25503 roughly a month later.
--
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 #28411: Fix calendar view for CronTriggerTimeTable dags
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28411:
URL: https://github.com/apache/airflow/pull/28411#discussion_r1055207870
##########
airflow/www/views.py:
##########
@@ -2828,7 +2828,7 @@ def _convert_to_date(session, column):
restriction = TimeRestriction(dag.start_date, dag.end_date, False)
dates = collections.Counter()
- if isinstance(dag.timetable, CronDataIntervalTimetable):
+ if isinstance(dag.timetable, CronTriggerTimetable):
Review Comment:
Ah I get it, because CronTriggerTimetable always uses the current time when catchup=False without considering `last_automated_data_interval`. It should; I’ll fix it in a different PR. (Edit: filed in #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