You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "weiwenlan (via GitHub)" <gi...@apache.org> on 2023/04/11 02:56:03 UTC
[GitHub] [airflow] weiwenlan opened a new pull request, #30571: first commit
weiwenlan opened a new pull request, #30571:
URL: https://github.com/apache/airflow/pull/30571
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ 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] hussein-awala commented on a diff in pull request #30571: Issue on the try_number is not in the loop
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #30571:
URL: https://github.com/apache/airflow/pull/30571#discussion_r1164687271
##########
airflow/sensors/base.py:
##########
@@ -251,7 +251,7 @@ def run_duration() -> float:
raise AirflowRescheduleException(reschedule_date)
else:
time.sleep(self._get_next_poke_interval(started_at, run_duration, try_number))
- try_number += 1
+ try_number += 1
Review Comment:
When self.reschedule is True, `raise AirflowRescheduleException(reschedule_date)` will stop the execution, and the `try_number += 1` is not reached even if it's outside the else block.
Even if we add try...finally, updating the try_number cannot solve the problem, because the try_number is resetting when we re-execute (reschedule) the sensor.
IMO, the try_number state should be stored somewhere in order to use it in the next reschedule (ex XCom)
--
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] weiwenlan commented on pull request #30571: Issue on the try_number is not in the loop
Posted by "weiwenlan (via GitHub)" <gi...@apache.org>.
weiwenlan commented on PR #30571:
URL: https://github.com/apache/airflow/pull/30571#issuecomment-1502632691
#30572
--
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] boring-cyborg[bot] commented on pull request #30571: first commit
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #30571:
URL: https://github.com/apache/airflow/pull/30571#issuecomment-1502617837
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (ruff, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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] weiwenlan commented on a diff in pull request #30571: Issue on the try_number is not in the loop
Posted by "weiwenlan (via GitHub)" <gi...@apache.org>.
weiwenlan commented on code in PR #30571:
URL: https://github.com/apache/airflow/pull/30571#discussion_r1163632571
##########
airflow/sensors/base.py:
##########
@@ -251,7 +251,7 @@ def run_duration() -> float:
raise AirflowRescheduleException(reschedule_date)
else:
time.sleep(self._get_next_poke_interval(started_at, run_duration, try_number))
- try_number += 1
+ try_number += 1
Review Comment:
Hi @hussein-awala, this is just put the try_number outside the if logic. For now, if reschedule is true, the `try_number` will always be 1 and the exponential_backoff will be of no use. The exponential_backoff parameter is used in the function: `_get_next_poke_interval` and it will use `try_number` to generate the backoff interval. If the `try_number` remains 1, the interval will not change at all, even if the exponential_backoff is set to True. Show some code scripts below.
```
# code in the execute()
if self.reschedule:
next_poke_interval = self._get_next_poke_interval(started_at, run_duration, try_number)
reschedule_date = timezone.utcnow() + timedelta(seconds=next_poke_interval)
if _is_metadatabase_mysql() and reschedule_date > _MYSQL_TIMESTAMP_MAX:
raise AirflowSensorTimeout(
f"Cannot reschedule DAG {log_dag_id} to {reschedule_date.isoformat()} "
f"since it is over MySQL's TIMESTAMP storage limit."
)
raise AirflowRescheduleException(reschedule_date)
else:
time.sleep(self._get_next_poke_interval(started_at, run_duration, try_number))
try_number += 1
```
```
# code about determine the poke interval
def _get_next_poke_interval(
self,
started_at: datetime.datetime | float,
run_duration: Callable[[], float],
try_number: int,
) -> float:
"""Using the similar logic which is used for exponential backoff retry delay for operators."""
if not self.exponential_backoff:
return self.poke_interval
min_backoff = int(self.poke_interval * (2 ** (try_number - 2)))
```
--
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 #30571: Issue on the try_number is not in the loop
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #30571:
URL: https://github.com/apache/airflow/pull/30571#discussion_r1174399124
##########
airflow/sensors/base.py:
##########
@@ -251,7 +251,7 @@ def run_duration() -> float:
raise AirflowRescheduleException(reschedule_date)
else:
time.sleep(self._get_next_poke_interval(started_at, run_duration, try_number))
- try_number += 1
+ try_number += 1
Review Comment:
Yes. This solution is by far not complete. I will close the PR - we have few others including #30669 from @hussein-awala and related issue as well. #18080
I beleive (correct me if I am wrong @hussein-awala @uranusjr ) but we are converging on the need to add a new entity to keep try_nums ?).
--
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 closed pull request #30571: Issue on the try_number is not in the loop
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk closed pull request #30571: Issue on the try_number is not in the loop
URL: https://github.com/apache/airflow/pull/30571
--
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] hussein-awala commented on a diff in pull request #30571: Issue on the try_number is not in the loop
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #30571:
URL: https://github.com/apache/airflow/pull/30571#discussion_r1163407143
##########
airflow/sensors/base.py:
##########
@@ -251,7 +251,7 @@ def run_duration() -> float:
raise AirflowRescheduleException(reschedule_date)
else:
time.sleep(self._get_next_poke_interval(started_at, run_duration, try_number))
- try_number += 1
+ try_number += 1
Review Comment:
@weiwenlan How this change can solve the problem?
--
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] utkarsharma2 commented on a diff in pull request #30571: Issue on the try_number is not in the loop
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #30571:
URL: https://github.com/apache/airflow/pull/30571#discussion_r1162401646
##########
airflow/sensors/base.py:
##########
@@ -251,7 +251,7 @@ def run_duration() -> float:
raise AirflowRescheduleException(reschedule_date)
else:
time.sleep(self._get_next_poke_interval(started_at, run_duration, try_number))
- try_number += 1
+ try_number += 1
Review Comment:
@weiwenlan It would be nice to see a test case asserting the expected poke interval to `time.sleep()` method for reschedule and non-reschedule modes. :)
--
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] weiwenlan commented on a diff in pull request #30571: Issue on the try_number is not in the loop
Posted by "weiwenlan (via GitHub)" <gi...@apache.org>.
weiwenlan commented on code in PR #30571:
URL: https://github.com/apache/airflow/pull/30571#discussion_r1163634812
##########
airflow/sensors/base.py:
##########
@@ -251,7 +251,7 @@ def run_duration() -> float:
raise AirflowRescheduleException(reschedule_date)
else:
time.sleep(self._get_next_poke_interval(started_at, run_duration, try_number))
- try_number += 1
+ try_number += 1
Review Comment:
@utkarsharma2 Thanks for the suggestion! I will try to add some test cases for the exponential_backoff parameter in different modes.
--
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