dstandish commented on code in PR #35887:
URL: https://github.com/apache/airflow/pull/35887#discussion_r1416631463


##########
airflow/timetables/_cron.py:
##########
@@ -32,21 +31,32 @@
     from pendulum import DateTime
 
 
-def _is_schedule_fixed(expression: str) -> bool:
-    """Figures out if the schedule has a fixed time (e.g. 3 AM every day).
+def _covers_every_hour(cron: croniter) -> bool:
+    """Check whether the given cron runs at least once an hour.
 
-    :return: True if the schedule has a fixed time, False if not.
+    This indicates whether we need to implement a workaround for (what I call)
+    the "fold hour problem". Folding happens when a region switches time
+    backwards, usually as a part of ending a DST period, causing a block of 
time
+    to occur twice in the wall clock. This is indicated by the ``fold`` flag on
+    datetime.
 
-    Detection is done by "peeking" the next two cron trigger time; if the
-    two times have the same minute and hour value, the schedule is fixed,
-    and we *don't* need to perform the DST fix.
+    As an example, Switzerland in 2023 ended DST on 3am (wall clock time, 
UTC+2)
+    by dialing back the clock to 2am (UTC+1). So for (say) ``30 * * * *``, if
+    the last run was 2:30am (UTC+2), the next needs to be 2:30am (UTC+1, 
folded)
+    instead of 3:30am.
 
-    This assumes DST happens on whole minute changes (e.g. 12:59 -> 12:00).
+    While this technically happens for all runs (in such a timezone), we only
+    really care about runs that happen at least once an hour, and can
+    provide a somewhat reasonable rationale to skip the fold hour for things
+    such as ``*/2`` (every two hour). So we try to *minially* peak into 
croniter
+    internals to work around the issue.

Review Comment:
   mainly this suggestion 
   
   (1) tries to clarify "happens for all runs" vs "happens for all cron 
schedules".  A given run is just a single instant in time but this is more 
about the schedule, i.e. computing next run from this run
   
   and (2) suggests to just state the rationale rather than saying "we can 
provide" but don't provide



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to