You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "mjsqu (via GitHub)" <gi...@apache.org> on 2023/08/24 23:48:06 UTC
[GitHub] [airflow] mjsqu opened a new pull request, #33712: Replace waiter_max_attempts default with sys.maxsize
mjsqu opened a new pull request, #33712:
URL: https://github.com/apache/airflow/pull/33712
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
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: #33711
related: #33016, #30586
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
Prior to v8.0.0 the default `waiter_max_attempts` parameter in `EcsRunTaskOperator` was set to `sys.maxsize`. The value is a large integer, which *should* always exceed the `execution_timeout` value for a given task. Changes in v8.0.0 introduced a configurable value defaulting to 100 for this parameter, with a delay defaulting to 6 seconds (the changes were introduced to resolve #30586). I believe the introduction of a default value of 100 has changed the behaviour of the operator - now when not specified, all tasks detach from the running ECS Task at 10 minutes, reporting failed status back to Airflow.
**^ 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] eladkal merged pull request #33712: Increase `waiter_max_attempts` default value in `EcsRunTaskOperator`
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal merged PR #33712:
URL: https://github.com/apache/airflow/pull/33712
--
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 #33712: Increase `waiter_max_attempts` default value in `EcsRunTaskOperator`
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #33712:
URL: https://github.com/apache/airflow/pull/33712#issuecomment-1698929785
Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions.
--
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] mjsqu commented on pull request #33712: Replace waiter_max_attempts default with sys.maxsize
Posted by "mjsqu (via GitHub)" <gi...@apache.org>.
mjsqu commented on PR #33712:
URL: https://github.com/apache/airflow/pull/33712#issuecomment-1694062240
> LGTM
>
> needs rebase and resolve conflicts
Resolved conflict in `CHANGELOG.rst` - 56782d5
--
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] eladkal commented on a diff in pull request #33712: Replace waiter_max_attempts default with sys.maxsize
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #33712:
URL: https://github.com/apache/airflow/pull/33712#discussion_r1305655656
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -476,7 +476,7 @@ def __init__(
number_logs_exception: int = 10,
wait_for_completion: bool = True,
waiter_delay: int = 6,
- waiter_max_attempts: int = 100,
+ waiter_max_attempts: int = sys.maxsize, # Unless specified timeout is managed by airflow
Review Comment:
> Agree it might be seen as breaking but looking at the problem description I think the previous change was accidentally breaking (i.e. changing from effectively infinite to finite) and this one is simply a bugfix restoring it.
Then I can accept this as non breaking change. Yet requesting to add a note explaining it in the top of the CHANGELOG. I will bake it to the proper release version during release process.
--
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] mjsqu commented on pull request #33712: Replace waiter_max_attempts default with sys.maxsize
Posted by "mjsqu (via GitHub)" <gi...@apache.org>.
mjsqu commented on PR #33712:
URL: https://github.com/apache/airflow/pull/33712#issuecomment-1694763838
Latest test failures:
```py
cluster="c", tasks=["arn"], WaiterConfig={"Delay": 6, "MaxAttempts": 1000000*365*24*60*10}
)
> assert 1000000*365*24*60*10 == client_mock.get_waiter.return_value.config.max_attempts
E AssertionError: assert ((((1000000 * 365) * 24) * 60) * 10) == <MagicMock name='client.get_waiter().config.max_attempts' id='139852189801392'>
E + where <MagicMock name='client.get_waiter().config.max_attempts' id='139852189801392'> = <MagicMock name='client.get_waiter().config' id='139852190309584'>.max_attempts
E + where <MagicMock name='client.get_waiter().config' id='139852190309584'> = <MagicMock name='client.get_waiter()' id='139852190269200'>.config
E + where <MagicMock name='client.get_waiter()' id='139852190269200'> = <MagicMock name='client.get_waiter' id='139852190252720'>.return_value
E + where <MagicMock name='client.get_waiter' id='139852190252720'> = <MagicMock name='client' id='139852190228688'>.get_waiter
```
Struggling to understand how the assertion fails, perhaps there's a precision issue such that `client_mock.get_waiter.return_value.config.max_attempts` does not return the exact value it was provided with?
--
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] mjsqu commented on a diff in pull request #33712: Replace waiter_max_attempts default with sys.maxsize
Posted by "mjsqu (via GitHub)" <gi...@apache.org>.
mjsqu commented on code in PR #33712:
URL: https://github.com/apache/airflow/pull/33712#discussion_r1306098314
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -476,7 +476,7 @@ def __init__(
number_logs_exception: int = 10,
wait_for_completion: bool = True,
waiter_delay: int = 6,
- waiter_max_attempts: int = 100,
+ waiter_max_attempts: int = sys.maxsize, # Unless specified timeout is managed by airflow
Review Comment:
Changelog note added [456ee50](https://github.com/apache/airflow/pull/33712/commits/456ee504884b21a94be52788501de1dba0e5bb02)
Unsure of the best way to refer back to the original PR that I am partially reverting, I trust you'll properly format the Changelog message when it comes to release time. TIA.
--
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] mjsqu commented on pull request #33712: Replace waiter_max_attempts default with sys.maxsize
Posted by "mjsqu (via GitHub)" <gi...@apache.org>.
mjsqu commented on PR #33712:
URL: https://github.com/apache/airflow/pull/33712#issuecomment-1694764636
Anyone following this PR/Issue, we have set the waiter_max_attempts value to `sys.maxsize` in the interim for affected `EcsRunTaskOperator` tasks, but you could choose a sufficiently large value to force the waiter to run for longer than `execution_timeout`:
```
waiter_max_attempts * waiter_delay > execution_timeout
```
e.g. for an `execution_timeout` of 1 hour (3600 seconds), set:
```py
waiter_max_attempts = 650, # Greater than 600
waiter_delay = 6,
# Total waiter time = 3,900 seconds == 65 minutes
```
--
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] eladkal commented on a diff in pull request #33712: Replace waiter_max_attempts default with sys.maxsize
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #33712:
URL: https://github.com/apache/airflow/pull/33712#discussion_r1305655656
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -476,7 +476,7 @@ def __init__(
number_logs_exception: int = 10,
wait_for_completion: bool = True,
waiter_delay: int = 6,
- waiter_max_attempts: int = 100,
+ waiter_max_attempts: int = sys.maxsize, # Unless specified timeout is managed by airflow
Review Comment:
> Agree it might be seen as breaking but looking at the problem description I think the previous change was accidentally breaking (i.e. changing from effectively infinite to finite) and this one is simply a bugfix restoring it.
Then I can accept this as non breaking change but a bug fix. Yet requesting to add a note explaining it in the top of the CHANGELOG. I will bake it to the proper release version during release process.
--
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] eladkal commented on a diff in pull request #33712: Replace waiter_max_attempts default with sys.maxsize
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #33712:
URL: https://github.com/apache/airflow/pull/33712#discussion_r1306175597
##########
airflow/providers/amazon/CHANGELOG.rst:
##########
@@ -26,6 +26,8 @@
Changelog
---------
+* ``Fix revert default waiter max attempts to sys.maxsize in 'EcsRunTaskOperator' (partial revert of #30586) (#33712)``
Review Comment:
Lets modify this to a more explanatory text.
Dont mention PR numbers. The commit title that we add during release cycle will do that and refer to this PR for more details.
What we are seeking in the note is just a simple explnatipn of the situation "A bug intoduced in provider version... Caused.... In this version we are fixing it by returning value to ..."
Something like this.
--
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 #33712: Replace waiter_max_attempts default with sys.maxsize
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #33712:
URL: https://github.com/apache/airflow/pull/33712#discussion_r1305482708
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -476,7 +476,7 @@ def __init__(
number_logs_exception: int = 10,
wait_for_completion: bool = True,
waiter_delay: int = 6,
- waiter_max_attempts: int = 100,
+ waiter_max_attempts: int = sys.maxsize, # Unless specified timeout is managed by airflow
Review Comment:
There are two things here:
1. Changing the default value to infinite could be a breaking change (cc: @eladkal )
2. If it's okay to change it, the best way is to make it None and replace the loop in the Trigger with `while True` when it's None.
--
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] mjsqu commented on a diff in pull request #33712: Replace waiter_max_attempts default with sys.maxsize
Posted by "mjsqu (via GitHub)" <gi...@apache.org>.
mjsqu commented on code in PR #33712:
URL: https://github.com/apache/airflow/pull/33712#discussion_r1305505482
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -476,7 +476,7 @@ def __init__(
number_logs_exception: int = 10,
wait_for_completion: bool = True,
waiter_delay: int = 6,
- waiter_max_attempts: int = 100,
+ waiter_max_attempts: int = sys.maxsize, # Unless specified timeout is managed by airflow
Review Comment:
Thanks for the review. I agree that adding `sys.maxsize` must have been a workaround and is a bit non-standard so perhaps this PR might be able to resolve that by using None.
I had considered if the `max_attempts` and `delay` parameters could be calculated at runtime by taking into consideration the `execution_timeout` value, however this would require a bit more of a rewrite of the Operator.
--
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] mjsqu commented on pull request #33712: Replace waiter_max_attempts default with sys.maxsize
Posted by "mjsqu (via GitHub)" <gi...@apache.org>.
mjsqu commented on PR #33712:
URL: https://github.com/apache/airflow/pull/33712#issuecomment-1692577110
While I wait for my `sleep 800` command to run having manually set `waiter_max_attempts` to `sys.maxsize`...
Here's the documentation for the ECS TasksStopped waiter: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/ecs/waiter/TasksStopped.html
If not set, it has defaults of 6,100 too.
--
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 #33712: Replace waiter_max_attempts default with sys.maxsize
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #33712:
URL: https://github.com/apache/airflow/pull/33712#issuecomment-1692556584
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).
- Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
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] mjsqu commented on pull request #33712: Replace waiter_max_attempts default with sys.maxsize
Posted by "mjsqu (via GitHub)" <gi...@apache.org>.
mjsqu commented on PR #33712:
URL: https://github.com/apache/airflow/pull/33712#issuecomment-1694563330
> Tests fail :(
Tests were failing due to the size of `sys.maxsize`. The `waiter_max_attempts` value is used in a timedelta calculation in the test suite somewhere:
```py
>>> timedelta(seconds=sys.maxsize * delay + 60)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
OverflowError: Python int too large to convert to C int
```
To resolve this, I have set the default `waiter_max_attempts` value high enough such that the waiter would run for 1M years. This allows the timedelta calculation to succeed, while providing a value that is likely longer than any ECS Tasks wil ever run for.
```py
max_attempts = 1000000*365*24*60*10
>>> timedelta(seconds=max_attempts * delay + 60)
datetime.timedelta(days=365000000, seconds=60)
```
This raises a question about why the `_wait_for_tasks_ended` function is necessary as the default behaviour we're trying to achieve circumvents it.
--
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 #33712: Replace waiter_max_attempts default with sys.maxsize
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #33712:
URL: https://github.com/apache/airflow/pull/33712#discussion_r1306727029
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -665,7 +665,6 @@ def _wait_for_task_ended(self) -> None:
return
waiter = self.client.get_waiter("tasks_stopped")
- waiter.config.max_attempts = sys.maxsize # timeout is managed by airflow
Review Comment:
The failed check was not valid after removing this line.
--
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 #33712: Replace waiter_max_attempts default with sys.maxsize
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #33712:
URL: https://github.com/apache/airflow/pull/33712#discussion_r1305493733
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -476,7 +476,7 @@ def __init__(
number_logs_exception: int = 10,
wait_for_completion: bool = True,
waiter_delay: int = 6,
- waiter_max_attempts: int = 100,
+ waiter_max_attempts: int = sys.maxsize, # Unless specified timeout is managed by airflow
Review Comment:
Agree it might be seen as breaking but looking at the problem description I think the previous change was accidentally breaking (i.e. changing from effectively infinite to finite) and this one is simply a bugfix restoring it.
--
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] eladkal commented on pull request #33712: Replace waiter_max_attempts default with sys.maxsize
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #33712:
URL: https://github.com/apache/airflow/pull/33712#issuecomment-1694490245
Tests fail :(
--
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] mjsqu commented on a diff in pull request #33712: Replace waiter_max_attempts default with sys.maxsize
Posted by "mjsqu (via GitHub)" <gi...@apache.org>.
mjsqu commented on code in PR #33712:
URL: https://github.com/apache/airflow/pull/33712#discussion_r1305505482
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -476,7 +476,7 @@ def __init__(
number_logs_exception: int = 10,
wait_for_completion: bool = True,
waiter_delay: int = 6,
- waiter_max_attempts: int = 100,
+ waiter_max_attempts: int = sys.maxsize, # Unless specified timeout is managed by airflow
Review Comment:
Thanks for the review. I agree that adding `sys.maxsize` must have been a workaround and is a bit non-standard so perhaps this PR might be able to resolve that by using some other method.
I had considered if the `max_attempts` and `delay` parameters could be calculated at runtime by taking into consideration the `execution_timeout` value, however this would require a bit more of a rewrite of the Operator.
--
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] eladkal commented on a diff in pull request #33712: Increase EcsRunOperator waiter_max_attempts default value
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #33712:
URL: https://github.com/apache/airflow/pull/33712#discussion_r1309817249
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -476,7 +475,13 @@ def __init__(
number_logs_exception: int = 10,
wait_for_completion: bool = True,
waiter_delay: int = 6,
- waiter_max_attempts: int = 100,
+ waiter_max_attempts: int = 1000000
+ * 365
+ * 24
+ * 60
+ * 10,
Review Comment:
```suggestion
waiter_max_attempts: int = 1000000 * 365 * 24 * 60 * 10,
```
To fix static checks
--
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