uranusjr commented on a change in pull request #19145:
URL: https://github.com/apache/airflow/pull/19145#discussion_r734198704
##########
File path: airflow/timetables/interval.py
##########
@@ -205,14 +205,14 @@ def _skip_to_latest(self, earliest: Optional[DateTime])
-> DateTime:
This is slightly different from the delta version at terminal values.
If the next schedule should start *right now*, we want the data
interval
- that start right now now, not the one that ends now.
+ that start now, not the one that ends now.
"""
current_time = DateTime.utcnow()
- next_start = self._get_next(current_time)
last_start = self._get_prev(current_time)
Review comment:
The previous implementation
```python
next_start = self._get_next(current_time)
last_start = self._get_prev(current_time)
```
had a bug if `current_time` falls right on the interval boundary (e.g. the
full hour mark for a `@hourly` schedule interval) because `croniter` would make
`next_start` and `last_start` _two_ hours apart instead of one. So this is
changed to
```python
last_start = self._get_prev(current_time)
next_start = self._get_next(last_start)
```
toe ensure the two starts are one hour apart, and `current_time ==
next_start` for the interval boundary case. This is not really practically
relevant (what's the chance a `now()` calls falls directly one that time with
microsecond accurary), but is an issue in unit tests.
##########
File path: airflow/timetables/interval.py
##########
@@ -205,14 +205,14 @@ def _skip_to_latest(self, earliest: Optional[DateTime])
-> DateTime:
This is slightly different from the delta version at terminal values.
If the next schedule should start *right now*, we want the data
interval
- that start right now now, not the one that ends now.
+ that start now, not the one that ends now.
"""
current_time = DateTime.utcnow()
- next_start = self._get_next(current_time)
last_start = self._get_prev(current_time)
Review comment:
The previous implementation
```python
next_start = self._get_next(current_time)
last_start = self._get_prev(current_time)
```
had a bug if `current_time` falls right on the interval boundary (e.g. the
full hour mark for a `@hourly` schedule interval) because `croniter` would make
`next_start` and `last_start` _two_ hours apart instead of one. So this is
changed to
```python
last_start = self._get_prev(current_time)
next_start = self._get_next(last_start)
```
toe ensure the two starts are one hour apart, and `current_time ==
next_start` for the interval boundary case. This is not really practically
relevant (what's the chance a `now()` call falls directly one that time with
microsecond accurary), but is an issue in unit 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
