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/02/16 19:49:47 UTC
[GitHub] [airflow] markhopson opened a new pull request #14263: Added retry to ECS Operator
markhopson opened a new pull request #14263:
URL: https://github.com/apache/airflow/pull/14263
<!--
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 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/master/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/master/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#issuecomment-794823569
[The Workflow run](https://github.com/apache/airflow/actions/runs/638021554) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] markhopson commented on pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
markhopson commented on pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#issuecomment-781006855
@turbaszek Thanks for the feedback. I also saw some checks fail (i.e. `CI Build / Postgres9.6,Py3.6`, `CI Build / MySQL5.7, Py3.6`, `CI Build / Sqlite Py3.6`) but I'm not sure what to do to fix them. Can you please advise me? Thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#issuecomment-808563627
Awesome work, congrats on your first merged pull request!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] markhopson commented on a change in pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
markhopson commented on a change in pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#discussion_r577260397
##########
File path: airflow/www/yarn-error.log
##########
@@ -0,0 +1,8373 @@
+Arguments:
+ /usr/bin/node /usr/share/yarn/bin/yarn.js install --frozen-lockfile
+
+PATH:
+ /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
+
+Yarn version:
+ 1.22.5
+
+Node version:
+ 10.23.1
+
+Platform:
Review comment:
Nope. Good catch. Ty.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#discussion_r589292633
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -484,6 +504,33 @@ def expand_role(self, role: str) -> str:
else:
return self.get_client_type("iam").get_role(RoleName=role)["Role"]["Arn"]
+ @staticmethod
+ def retry(fun: Callable):
Review comment:
If we change this to
```python
def retry(should_retry: Callable[[Exception], bool], fun: Callable)
```
Then the `retry_if_permissible_error` can move out of BaseAWS in to ECSHook, used like this:
```python
def should_retry(exception: Exception):
"""Check if exception is related to ECS resource quota (CPU, MEM)."""
return isinstance(exception, ECSOperatorError) and any(
quota_reason in failure['reason']
for quota_reason in ['RESOURCE:MEMORY', 'RESOURCE:CPU']
for failure in exception.failures
)
...
@AwsBaseHook.retry(should_retry)
def _start_task(self):
```
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -484,6 +504,33 @@ def expand_role(self, role: str) -> str:
else:
return self.get_client_type("iam").get_role(RoleName=role)["Role"]["Arn"]
+ @staticmethod
+ def retry(fun: Callable):
+ """
+ A decorator that provides a mechanism to repeat requests in response to exceeding a temporary quote
+ limit.
+ """
+
+ def decorator_f(self):
Review comment:
```suggestion
@functools.wraps
def decorator_f(self, *args, **kwargs):
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#issuecomment-792637660
@markhopson See the review I just left -- I think that gives us a way to have the base retry functionality in the base hook, without having to put any service specific login in there. WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] markhopson commented on a change in pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
markhopson commented on a change in pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#discussion_r584223206
##########
File path: airflow/providers/amazon/aws/models/exceptions.py
##########
@@ -0,0 +1,29 @@
+#
+# 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.
+#
+# Note: Any AirflowException raised is expected to cause the TaskInstance
+# to be marked in an ERROR state
+
+
+class ECSOperatorError(Exception):
+ """Raise when ECS cannot handle the request."""
+
+ def __init__(self, failures: list, message: str):
+ self.failures = failures
+ self.message = message
+ super().__init__(self.message)
Review comment:
Changed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] markhopson commented on pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
markhopson commented on pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#issuecomment-794776484
> @markhopson See the review I just left -- I think that gives us a way to have the base retry functionality in the base hook, without having to put any service specific login in there. WDYT?
@ashb Good tip. I like it. I've made the change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] markhopson commented on a change in pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
markhopson commented on a change in pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#discussion_r584223674
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -484,6 +509,34 @@ def expand_role(self, role: str) -> str:
else:
return self.get_client_type("iam").get_role(RoleName=role)["Role"]["Arn"]
+ @staticmethod
+ def retry(fun: Callable):
+ """
+ A decorator that provides a mechanism to repeat requests in response to exceeding a temporary quote
+ limit.
+ """
+
+ def decorator_f(self):
+ quota_retry = getattr(self, 'quota_retry', None)
Review comment:
Good point. Hm I've changed it to `retry_args` to be more generic, but I'm open to any suggestions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb merged pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #14263:
URL: https://github.com/apache/airflow/pull/14263
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#issuecomment-803173557
TestECSOperator.test_execute_with_failures is now failing becuase the type of the exception it throws has changed -- I think that's fine, and the tests just need updating.
I haven't look in detail at the code/test though.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] markhopson commented on pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
markhopson commented on pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#issuecomment-791841984
@ashb Friendly bump here
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#issuecomment-791906968
Thanks, I'll take a look on Monday
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] markhopson commented on a change in pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
markhopson commented on a change in pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#discussion_r584228234
##########
File path: airflow/providers/amazon/aws/operators/ecs.py
##########
@@ -150,6 +151,7 @@ def __init__(
awslogs_region: Optional[str] = None,
awslogs_stream_prefix: Optional[str] = None,
propagate_tags: Optional[str] = None,
+ quota_retry: Optional[dict] = None,
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#issuecomment-795685234
[The Workflow run](https://github.com/apache/airflow/actions/runs/639912258) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#issuecomment-780077724
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/master/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/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/master/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/master/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/master/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] markhopson commented on a change in pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
markhopson commented on a change in pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#discussion_r590956126
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -484,6 +504,33 @@ def expand_role(self, role: str) -> str:
else:
return self.get_client_type("iam").get_role(RoleName=role)["Role"]["Arn"]
+ @staticmethod
+ def retry(fun: Callable):
Review comment:
I wish I thought of this. I've made this change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#issuecomment-802954632
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] markhopson commented on pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
markhopson commented on pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#issuecomment-799063573
@ashb Friendly bump.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] markhopson commented on a change in pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
markhopson commented on a change in pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#discussion_r584223155
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -484,6 +509,34 @@ def expand_role(self, role: str) -> str:
else:
return self.get_client_type("iam").get_role(RoleName=role)["Role"]["Arn"]
+ @staticmethod
+ def retry(fun: Callable):
+ """
+ A decorator that provides a mechanism to repeat requests in response to exceeding a temporary quote
+ limit.
+ """
+
+ def decorator_f(self):
+ quota_retry = getattr(self, 'quota_retry', None)
+ if quota_retry is None:
+ return fun(self)
+ multiplier = quota_retry.get('multiplier', 1)
+ min_limit = quota_retry.get('min', 1)
+ max_limit = quota_retry.get('max', 1)
+ stop_after_delay = quota_retry.get('stop_after_delay', 10)
+ logger = quota_retry.get('logger')
Review comment:
Hm I think think this was a mistake. I believe I was testing something with this. I will change this to use `self.logger` and not let user specify the logger in `quota_retry`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#discussion_r577170275
##########
File path: airflow/www/yarn-error.log
##########
@@ -0,0 +1,8373 @@
+Arguments:
+ /usr/bin/node /usr/share/yarn/bin/yarn.js install --frozen-lockfile
+
+PATH:
+ /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
+
+Yarn version:
+ 1.22.5
+
+Node version:
+ 10.23.1
+
+Platform:
Review comment:
Is this file supposed to be a part fo this PR?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#discussion_r583740272
##########
File path: airflow/providers/amazon/aws/operators/ecs.py
##########
@@ -27,6 +27,7 @@
from airflow.models import BaseOperator
from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
from airflow.providers.amazon.aws.hooks.logs import AwsLogsHook
+from airflow.providers.amazon.aws.models.exceptions import ECSOperatorError
Review comment:
```suggestion
from airflow.providers.amazon.aws.exceptions import ECSOperatorError
```
I think would be better name.
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -40,8 +41,32 @@
from airflow.exceptions import AirflowException
from airflow.hooks.base import BaseHook
from airflow.models.connection import Connection
+from airflow.providers.amazon.aws.models.exceptions import ECSOperatorError
from airflow.utils.log.logging_mixin import LoggingMixin
+ECS_QUOTA_ERROR_REASONS = [
Review comment:
Since these are ECS specific they shouldn't really be in the base_aws module.
##########
File path: airflow/providers/amazon/aws/models/exceptions.py
##########
@@ -0,0 +1,29 @@
+#
+# 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.
+#
+# Note: Any AirflowException raised is expected to cause the TaskInstance
+# to be marked in an ERROR state
+
+
+class ECSOperatorError(Exception):
+ """Raise when ECS cannot handle the request."""
+
+ def __init__(self, failures: list, message: str):
+ self.failures = failures
+ self.message = message
+ super().__init__(self.message)
Review comment:
```suggestion
super().__init__(message)
```
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -484,6 +509,34 @@ def expand_role(self, role: str) -> str:
else:
return self.get_client_type("iam").get_role(RoleName=role)["Role"]["Arn"]
+ @staticmethod
+ def retry(fun: Callable):
+ """
+ A decorator that provides a mechanism to repeat requests in response to exceeding a temporary quote
+ limit.
+ """
+
+ def decorator_f(self):
+ quota_retry = getattr(self, 'quota_retry', None)
+ if quota_retry is None:
+ return fun(self)
+ multiplier = quota_retry.get('multiplier', 1)
+ min_limit = quota_retry.get('min', 1)
+ max_limit = quota_retry.get('max', 1)
+ stop_after_delay = quota_retry.get('stop_after_delay', 10)
+ logger = quota_retry.get('logger')
Review comment:
Why not self.logger?
##########
File path: airflow/providers/amazon/aws/operators/ecs.py
##########
@@ -150,6 +151,7 @@ def __init__(
awslogs_region: Optional[str] = None,
awslogs_stream_prefix: Optional[str] = None,
propagate_tags: Optional[str] = None,
+ quota_retry: Optional[dict] = None,
Review comment:
Please add docs for this parameter.
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -484,6 +509,34 @@ def expand_role(self, role: str) -> str:
else:
return self.get_client_type("iam").get_role(RoleName=role)["Role"]["Arn"]
+ @staticmethod
+ def retry(fun: Callable):
+ """
+ A decorator that provides a mechanism to repeat requests in response to exceeding a temporary quote
+ limit.
+ """
+
+ def decorator_f(self):
+ quota_retry = getattr(self, 'quota_retry', None)
Review comment:
Not sure about this parameter name -- it seems very specific to EcsOperator only.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] markhopson commented on a change in pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
markhopson commented on a change in pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#discussion_r590966717
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -484,6 +504,33 @@ def expand_role(self, role: str) -> str:
else:
return self.get_client_type("iam").get_role(RoleName=role)["Role"]["Arn"]
+ @staticmethod
+ def retry(fun: Callable):
+ """
+ A decorator that provides a mechanism to repeat requests in response to exceeding a temporary quote
+ limit.
+ """
+
+ def decorator_f(self):
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] markhopson commented on a change in pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
markhopson commented on a change in pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#discussion_r584222761
##########
File path: airflow/providers/amazon/aws/operators/ecs.py
##########
@@ -27,6 +27,7 @@
from airflow.models import BaseOperator
from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
from airflow.providers.amazon.aws.hooks.logs import AwsLogsHook
+from airflow.providers.amazon.aws.models.exceptions import ECSOperatorError
Review comment:
Agreed. Changed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] markhopson commented on a change in pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
markhopson commented on a change in pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#discussion_r584228618
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -40,8 +41,32 @@
from airflow.exceptions import AirflowException
from airflow.hooks.base import BaseHook
from airflow.models.connection import Connection
+from airflow.providers.amazon.aws.models.exceptions import ECSOperatorError
from airflow.utils.log.logging_mixin import LoggingMixin
+ECS_QUOTA_ERROR_REASONS = [
Review comment:
Not sure what to do here. It feels weird to move this to `ecs.py` if I have code in `base_aws.py` that catches `ECSOperatorError`. For now, I've moved this variable into the `is_permissible_error()` scope to clarify intent. Any suggestions are welcomed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] markhopson commented on pull request #14263: Added retry to ECS Operator
Posted by GitBox <gi...@apache.org>.
markhopson commented on pull request #14263:
URL: https://github.com/apache/airflow/pull/14263#issuecomment-786722246
@turbaszek just a friendly bump to see what the next steps are here
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org