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 2021/08/13 09:43:13 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request #17601: Fix triggerer query where limit is not supported in some Mysql version
ephraimbuddy opened a new pull request #17601:
URL: https://github.com/apache/airflow/pull/17601
This PR fixes the triggerrer query where limit is not supported in some DB version
---
**^ 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 change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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] ephraimbuddy commented on a change in pull request #17601: Fix triggerer query where limit is not supported in some DB versions
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17601:
URL: https://github.com/apache/airflow/pull/17601#discussion_r688630038
##########
File path: airflow/triggers/temporal.py
##########
@@ -54,7 +54,7 @@ async def run(self):
unexpectedly, or handles a DST change poorly.
"""
# Sleep an hour at a time while it's more than 2 hours away
- while (self.moment - timezone.utcnow()).total_hours() > 2:
+ while ((self.moment - timezone.utcnow()).total_seconds() / 3600) > 2:
Review comment:
The test fails on AttributeError complaining that there’s no total_hours on a timedelta
--
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] andrewgodwin commented on a change in pull request #17601: Fix triggerer query where limit is not supported in some DB versions
Posted by GitBox <gi...@apache.org>.
andrewgodwin commented on a change in pull request #17601:
URL: https://github.com/apache/airflow/pull/17601#discussion_r688632301
##########
File path: airflow/triggers/temporal.py
##########
@@ -54,7 +54,7 @@ async def run(self):
unexpectedly, or handles a DST change poorly.
"""
# Sleep an hour at a time while it's more than 2 hours away
- while (self.moment - timezone.utcnow()).total_hours() > 2:
+ while ((self.moment - timezone.utcnow()).total_seconds() / 3600) > 2:
Review comment:
Strange... when I ran it locally it was a `pendulum.Period` rather than a `datetime.timedelta`. Let me try it locally again.
--
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] kaxil merged pull request #17601: Fix triggerer query where limit is not supported in some DB versions
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #17601:
URL: https://github.com/apache/airflow/pull/17601
--
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] ephraimbuddy commented on a change in pull request #17601: Fix triggerer query where limit is not supported in some DB versions
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17601:
URL: https://github.com/apache/airflow/pull/17601#discussion_r688629195
##########
File path: airflow/triggers/temporal.py
##########
@@ -54,7 +54,8 @@ async def run(self):
unexpectedly, or handles a DST change poorly.
"""
# Sleep an hour at a time while it's more than 2 hours away
- while (self.moment - timezone.utcnow()).total_hours() > 2:
+ hours_until_moment = (self.moment - timezone.utcnow()).total_seconds() / 3600
Review comment:
Thanks. That’s helpful!!
--
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] ephraimbuddy commented on pull request #17601: Fix triggerer query where limit is not supported in some DB versions
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #17601:
URL: https://github.com/apache/airflow/pull/17601#issuecomment-898473265
cc: @andrewgodwin
--
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] ephraimbuddy commented on a change in pull request #17601: Fix triggerer query where limit is not supported in some DB versions
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17601:
URL: https://github.com/apache/airflow/pull/17601#discussion_r688642166
##########
File path: airflow/triggers/temporal.py
##########
@@ -54,7 +54,7 @@ async def run(self):
unexpectedly, or handles a DST change poorly.
"""
# Sleep an hour at a time while it's more than 2 hours away
- while (self.moment - timezone.utcnow()).total_hours() > 2:
+ while ((self.moment - timezone.utcnow()).total_seconds() / 3600) > 2:
Review comment:
Cool!
--
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] andrewgodwin commented on a change in pull request #17601: Fix triggerer query where limit is not supported in some DB versions
Posted by GitBox <gi...@apache.org>.
andrewgodwin commented on a change in pull request #17601:
URL: https://github.com/apache/airflow/pull/17601#discussion_r688621109
##########
File path: airflow/triggers/temporal.py
##########
@@ -54,7 +54,8 @@ async def run(self):
unexpectedly, or handles a DST change poorly.
"""
# Sleep an hour at a time while it's more than 2 hours away
- while (self.moment - timezone.utcnow()).total_hours() > 2:
+ hours_until_moment = (self.moment - timezone.utcnow()).total_seconds() / 3600
Review comment:
You can't move this out of the loop - it will only evaluate once and then you have the risk of an infinite loop. It needs to evaluate in the loop's guard directly.
--
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] andrewgodwin commented on a change in pull request #17601: Fix triggerer query where limit is not supported in some DB versions
Posted by GitBox <gi...@apache.org>.
andrewgodwin commented on a change in pull request #17601:
URL: https://github.com/apache/airflow/pull/17601#discussion_r688628573
##########
File path: airflow/triggers/temporal.py
##########
@@ -54,7 +54,7 @@ async def run(self):
unexpectedly, or handles a DST change poorly.
"""
# Sleep an hour at a time while it's more than 2 hours away
- while (self.moment - timezone.utcnow()).total_hours() > 2:
+ while ((self.moment - timezone.utcnow()).total_seconds() / 3600) > 2:
Review comment:
Why make this change? I'm curious.
--
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] andrewgodwin commented on a change in pull request #17601: Fix triggerer query where limit is not supported in some DB versions
Posted by GitBox <gi...@apache.org>.
andrewgodwin commented on a change in pull request #17601:
URL: https://github.com/apache/airflow/pull/17601#discussion_r688635602
##########
File path: airflow/triggers/temporal.py
##########
@@ -54,7 +54,7 @@ async def run(self):
unexpectedly, or handles a DST change poorly.
"""
# Sleep an hour at a time while it's more than 2 hours away
- while (self.moment - timezone.utcnow()).total_hours() > 2:
+ while ((self.moment - timezone.utcnow()).total_seconds() / 3600) > 2:
Review comment:
Hah, looks like a change on `main` in the last few days means it's now a timedelta, odd! I would suggest moving the `/3600` over to become a `2*3600` though, for readability and less nesting?
--
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