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 2022/07/29 21:26:51 UTC
[GitHub] [airflow] ferruzzi opened a new pull request, #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
ferruzzi opened a new pull request, #25413:
URL: https://github.com/apache/airflow/pull/25413
The current EcsOperator is monolithic, does not have a hook, nor any sensors. This PR adds an ECS hook, modifies the existing EcsOperator to use it, adds a handful of other operators and sensors. The functionality of the existing EcsOperator was changed as little as possible. A future refactor of that operator might not be a bad idea, but adding the hook and sensors is the main goal of this PR.
Other changes:
- Moved some methods from the EcsOperator module into the new EcsHook module as that felt like a better home for them.
- Removes some old code that has been deprecated for multiple major releases.
- `airflow/contrib/operators/ecs_operator.py` was deprecated over 2 years ago
- `tests/providers/amazon/aws/operators/test_ecs_system.py` is also a few years old and not a proper system test by current standards.
- Deprecates the name `EcsOperator` in favor of `EcsRunTaskOperator` to match the standard naming conventions since there are now multiple ECS-related operators.
closes: https://github.com/apache/airflow/issues/24826
--
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 #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r933724461
##########
airflow/contrib/operators/ecs_operator.py:
##########
@@ -1,30 +0,0 @@
-#
Review Comment:
This file can be removed only in Airflow 3 so we can not remove it yet.
--
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] ferruzzi commented on pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on PR #25413:
URL: https://github.com/apache/airflow/pull/25413#issuecomment-1202987424
> There are several places where code is removed. We should deprecate first
Unless I made a mistake, I only removed code which has been deprecated for quite some time.
--
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] ferruzzi commented on pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on PR #25413:
URL: https://github.com/apache/airflow/pull/25413#issuecomment-1209707067
Alright, it just went green, so at least the CI is happy.
@jarek - My main reason for wanting it in this release was because it's removing deprecated code. Based on a chat with @eladkal, it doesn't sound like he thinks that's such a big deal. If you both think that isn't a good enough reason to hold up the release then by all means push on without 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] potiuk commented on a diff in pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r936030041
##########
airflow/contrib/operators/ecs_operator.py:
##########
@@ -1,30 +0,0 @@
-#
Review Comment:
I have an idea for the future. Should we move alll the deprecated/contrib operators from 1.10 out to a separate "apache-contrib" package (when we split out providers).
That will allow to get rid of the contrib, and get rid of many 1.10 deprecations out of airlfow main repo at least (it can still be a dependency of Airflow package so that it is installed automatically when airflow is installed). @ashb @kaxil 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r938997160
##########
airflow/contrib/operators/ecs_operator.py:
##########
@@ -1,30 +0,0 @@
-#
Review Comment:
I was a bit confused on how that chain of depredations should play out, but now with https://github.com/apache/airflow/pull/25543 I'm doubly confused on how the two will interact.
--
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] ferruzzi commented on a diff in pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r938994761
##########
airflow/contrib/operators/ecs_operator.py:
##########
@@ -1,30 +0,0 @@
-#
Review Comment:
ACK. ~Will rebase and do a force-push.~ Done.
Thanks to both of you for the help.
--
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 #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r933724461
##########
airflow/contrib/operators/ecs_operator.py:
##########
@@ -1,30 +0,0 @@
-#
Review Comment:
This file can be removed only in Airflow 3 as its part of Airflow core so we can not remove it yet.
--
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] ferruzzi commented on pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on PR #25413:
URL: https://github.com/apache/airflow/pull/25413#issuecomment-1209881566
@potiuk Rebase done, passes all CI and static tests locally
--
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 merged pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #25413:
URL: https://github.com/apache/airflow/pull/25413
--
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 #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #25413:
URL: https://github.com/apache/airflow/pull/25413#issuecomment-1209702803
> If the tests pass, do you think we should be fine with releasing it in this upcoming wave of providers?
I wont have time today to review the code.. i didnt do full review when I left the comment.
If the PR is OK from your side dont let me get in the way :)
I can chime in tommorow if my review is required.
--
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] ferruzzi commented on a diff in pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r936086187
##########
airflow/contrib/operators/ecs_operator.py:
##########
@@ -1,30 +0,0 @@
-#
Review Comment:
> As for your question - in this file you need to support only classes that existed. We dont add new functionality
I thought I understood this, but the CI is proving me wrong. Before this CR, contrib looked like this:
```
from airflow.providers.amazon.aws.operators.ecs import ECSOperator, ECSProtocol
__all__ = ["ECSOperator", "ECSProtocol"]
warnings.warn(
"This module is deprecated. Please use `airflow.providers.amazon.aws.operators.ecs`.",
DeprecationWarning,
stacklevel=2,
)
```
and `deprecated_classes.py` had this:
```
(
'airflow.providers.amazon.aws.operators.ecs.ECSOperator',
'airflow.contrib.operators.ecs_operator.ECSOperator',
),
```
ECSOperator and ECSProtocol were deprecated some time ago in favor of the names EcsOperator and EcsProtocol, so it was importing classes which were deprecated. Do those deprecated classes still have to stay now as well? It has been a full major release of the provider package so I wanted to start removing those old methods. In this PR I am now renaming EcsOperator to EcsRunTaskOperator, and EcsProtocol has been deprecated and moved into *.hooks.ecs.EcsProtocol
--
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] zachliu commented on a diff in pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
zachliu commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r956520828
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -17,160 +17,257 @@
# under the License.
import re
import sys
-import time
-from collections import deque
-from datetime import datetime, timedelta
-from logging import Logger
-from threading import Event, Thread
-from typing import Dict, Generator, Optional, Sequence
+import warnings
+from datetime import timedelta
+from typing import TYPE_CHECKING, Dict, List, Optional, Sequence
-from botocore.exceptions import ClientError, ConnectionClosedError
-from botocore.waiter import Waiter
+import boto3
+from airflow.compat.functools import cached_property
from airflow.exceptions import AirflowException
from airflow.models import BaseOperator, XCom
from airflow.providers.amazon.aws.exceptions import EcsOperatorError, EcsTaskFailToStart
+
+# TODO: Remove the following import when EcsProtocol and EcsTaskLogFetcher deprecations are removed.
+from airflow.providers.amazon.aws.hooks import ecs
from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
-from airflow.providers.amazon.aws.hooks.logs import AwsLogsHook
-from airflow.typing_compat import Protocol, runtime_checkable
+from airflow.providers.amazon.aws.hooks.ecs import (
+ EcsClusterStates,
+ EcsHook,
+ EcsTaskDefinitionStates,
+ should_retry_eni,
+)
+from airflow.providers.amazon.aws.sensors.ecs import EcsClusterStateSensor, EcsTaskDefinitionStateSensor
from airflow.utils.session import provide_session
+if TYPE_CHECKING:
+ from airflow.utils.context import Context
-def should_retry(exception: Exception):
- """Check if exception is related to ECS resource quota (CPU, MEM)."""
- if isinstance(exception, EcsOperatorError):
- return any(
- quota_reason in failure['reason']
- for quota_reason in ['RESOURCE:MEMORY', 'RESOURCE:CPU']
- for failure in exception.failures
- )
- return False
+DEFAULT_CONN_ID = 'aws_default'
-def should_retry_eni(exception: Exception):
- """Check if exception is related to ENI (Elastic Network Interfaces)."""
- if isinstance(exception, EcsTaskFailToStart):
- return any(
- eni_reason in exception.message
- for eni_reason in ['network interface provisioning', 'ResourceInitializationError']
- )
- return False
+class EcsBaseOperator(BaseOperator):
+ """This is the base operator for all Elastic Container Service operators."""
+
+ def __init__(self, **kwargs):
+ self.aws_conn_id = kwargs.get('aws_conn_id', DEFAULT_CONN_ID)
+ self.region = kwargs.get('region')
+ super().__init__(**kwargs)
Review Comment:
nvm, https://github.com/apache/airflow/pull/25989
--
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 #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r938993941
##########
airflow/contrib/operators/ecs_operator.py:
##########
@@ -1,30 +0,0 @@
-#
Review Comment:
I can take this one. But you need to rebase @ferruzzi - this one has problems from few days back when we fixed Flask 2.2 compatibility:
--
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] ferruzzi commented on a diff in pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r935831292
##########
airflow/contrib/operators/ecs_operator.py:
##########
@@ -1,30 +0,0 @@
-#
Review Comment:
But it's so olllllldddddddd. Git Blame shows that it was deprecated at least 2 years ago. If it needs to stay, I'll add it back.
--
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 #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r938986510
##########
airflow/contrib/operators/ecs_operator.py:
##########
@@ -1,30 +0,0 @@
-#
Review Comment:
It will take me a few days to get to 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] potiuk commented on a diff in pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r937572740
##########
airflow/contrib/operators/ecs_operator.py:
##########
@@ -1,30 +0,0 @@
-#
Review Comment:
Well. They shoudl be. But this is not a must actually.
Contrib were part of Airflow 1.10. When we moved to 2, we vowed to keep MOSTLY compatibility with 1.10. I.e. people who still used 1.10 would be able to continue using them. There were a number of exceptions and back then we had "backport" providers so people could simply move to use backports "airfow.providers.*" but we stoppped (as planned) releasing those, so the "late-comers" will have to deal with conesequences ot potentially broken compatibility if they still use contrib.
Since then many of those Contrib operators and hooks were modified and they are not even backwards-compatible with the original Contrib ones.
So technically speaking, if an operator is anyhow backwards incompatible we do not have to keep it in contrib. But if it largely is, it makes it easier for user to have smooth migration.
--
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 pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25413:
URL: https://github.com/apache/airflow/pull/25413#issuecomment-1209694943
If the tests pass, do you think we should be fine with releasing it in this upcoming wave of providers?
--
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] ferruzzi commented on pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on PR #25413:
URL: https://github.com/apache/airflow/pull/25413#issuecomment-1200068496
Sorry about that, all passing now. :+1:
--
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 #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r935902816
##########
airflow/contrib/operators/ecs_operator.py:
##########
@@ -1,30 +0,0 @@
-#
Review Comment:
Regardless contrib files cant not be remove. In this specific case we must be backward compatible till Airflow 3
As for your question - in this file you need to support only classes that existed. We dont add new functionality
--
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 #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r935902816
##########
airflow/contrib/operators/ecs_operator.py:
##########
@@ -1,30 +0,0 @@
-#
Review Comment:
Regardless contrib files cant not be removed. In this specific case we must be backward compatible till Airflow 3
As for your question - in this file you need to support only classes that existed. We dont add new functionality
--
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] zachliu commented on a diff in pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
zachliu commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r956473322
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -17,160 +17,257 @@
# under the License.
import re
import sys
-import time
-from collections import deque
-from datetime import datetime, timedelta
-from logging import Logger
-from threading import Event, Thread
-from typing import Dict, Generator, Optional, Sequence
+import warnings
+from datetime import timedelta
+from typing import TYPE_CHECKING, Dict, List, Optional, Sequence
-from botocore.exceptions import ClientError, ConnectionClosedError
-from botocore.waiter import Waiter
+import boto3
+from airflow.compat.functools import cached_property
from airflow.exceptions import AirflowException
from airflow.models import BaseOperator, XCom
from airflow.providers.amazon.aws.exceptions import EcsOperatorError, EcsTaskFailToStart
+
+# TODO: Remove the following import when EcsProtocol and EcsTaskLogFetcher deprecations are removed.
+from airflow.providers.amazon.aws.hooks import ecs
from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
-from airflow.providers.amazon.aws.hooks.logs import AwsLogsHook
-from airflow.typing_compat import Protocol, runtime_checkable
+from airflow.providers.amazon.aws.hooks.ecs import (
+ EcsClusterStates,
+ EcsHook,
+ EcsTaskDefinitionStates,
+ should_retry_eni,
+)
+from airflow.providers.amazon.aws.sensors.ecs import EcsClusterStateSensor, EcsTaskDefinitionStateSensor
from airflow.utils.session import provide_session
+if TYPE_CHECKING:
+ from airflow.utils.context import Context
-def should_retry(exception: Exception):
- """Check if exception is related to ECS resource quota (CPU, MEM)."""
- if isinstance(exception, EcsOperatorError):
- return any(
- quota_reason in failure['reason']
- for quota_reason in ['RESOURCE:MEMORY', 'RESOURCE:CPU']
- for failure in exception.failures
- )
- return False
+DEFAULT_CONN_ID = 'aws_default'
-def should_retry_eni(exception: Exception):
- """Check if exception is related to ENI (Elastic Network Interfaces)."""
- if isinstance(exception, EcsTaskFailToStart):
- return any(
- eni_reason in exception.message
- for eni_reason in ['network interface provisioning', 'ResourceInitializationError']
- )
- return False
+class EcsBaseOperator(BaseOperator):
+ """This is the base operator for all Elastic Container Service operators."""
+
+ def __init__(self, **kwargs):
+ self.aws_conn_id = kwargs.get('aws_conn_id', DEFAULT_CONN_ID)
+ self.region = kwargs.get('region')
+ super().__init__(**kwargs)
Review Comment:
oh man, you passed all kwargs including the `aws_conn_id` to the BaseOperator
now 80% of my dags (i use EcsOperator heavily) are failing with
```
Broken DAG: [/usr/local/airflow/dags/my_dag.py] Traceback (most recent call last):
File "/usr/local/airflow/.local/lib/python3.8/site-packages/airflow/models/baseoperator.py", line 390, in apply_defaults
result = func(self, **kwargs, default_args=default_args)
File "/usr/local/airflow/.local/lib/python3.8/site-packages/airflow/models/baseoperator.py", line 743, in __init__
raise AirflowException(
airflow.exceptions.AirflowException: Invalid arguments were passed to EcsOperator (task_id: my-task). Invalid arguments were:
**kwargs: {'aws_conn_id': 'aws_default'}
```
is there a fix? will 2 simple pops do the job?
--
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 pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25413:
URL: https://github.com/apache/airflow/pull/25413#issuecomment-1209801816
Hmm. Some conflicts after merging #25543 - will you rebase @ferruzzi ?
--
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] ferruzzi commented on pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on PR #25413:
URL: https://github.com/apache/airflow/pull/25413#issuecomment-1199974445
Looks like I missed removing a deprecation somewhere, looking into 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] zachliu commented on a diff in pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
zachliu commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r956473322
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -17,160 +17,257 @@
# under the License.
import re
import sys
-import time
-from collections import deque
-from datetime import datetime, timedelta
-from logging import Logger
-from threading import Event, Thread
-from typing import Dict, Generator, Optional, Sequence
+import warnings
+from datetime import timedelta
+from typing import TYPE_CHECKING, Dict, List, Optional, Sequence
-from botocore.exceptions import ClientError, ConnectionClosedError
-from botocore.waiter import Waiter
+import boto3
+from airflow.compat.functools import cached_property
from airflow.exceptions import AirflowException
from airflow.models import BaseOperator, XCom
from airflow.providers.amazon.aws.exceptions import EcsOperatorError, EcsTaskFailToStart
+
+# TODO: Remove the following import when EcsProtocol and EcsTaskLogFetcher deprecations are removed.
+from airflow.providers.amazon.aws.hooks import ecs
from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
-from airflow.providers.amazon.aws.hooks.logs import AwsLogsHook
-from airflow.typing_compat import Protocol, runtime_checkable
+from airflow.providers.amazon.aws.hooks.ecs import (
+ EcsClusterStates,
+ EcsHook,
+ EcsTaskDefinitionStates,
+ should_retry_eni,
+)
+from airflow.providers.amazon.aws.sensors.ecs import EcsClusterStateSensor, EcsTaskDefinitionStateSensor
from airflow.utils.session import provide_session
+if TYPE_CHECKING:
+ from airflow.utils.context import Context
-def should_retry(exception: Exception):
- """Check if exception is related to ECS resource quota (CPU, MEM)."""
- if isinstance(exception, EcsOperatorError):
- return any(
- quota_reason in failure['reason']
- for quota_reason in ['RESOURCE:MEMORY', 'RESOURCE:CPU']
- for failure in exception.failures
- )
- return False
+DEFAULT_CONN_ID = 'aws_default'
-def should_retry_eni(exception: Exception):
- """Check if exception is related to ENI (Elastic Network Interfaces)."""
- if isinstance(exception, EcsTaskFailToStart):
- return any(
- eni_reason in exception.message
- for eni_reason in ['network interface provisioning', 'ResourceInitializationError']
- )
- return False
+class EcsBaseOperator(BaseOperator):
+ """This is the base operator for all Elastic Container Service operators."""
+
+ def __init__(self, **kwargs):
+ self.aws_conn_id = kwargs.get('aws_conn_id', DEFAULT_CONN_ID)
+ self.region = kwargs.get('region')
+ super().__init__(**kwargs)
Review Comment:
oh man, you passed all kwargs including the `aws_conn_id` to the BaseOperator
now 80% of my dags (i use EcsOperator heavily) are failing with
```
Broken DAG: [/usr/local/airflow/dags/my_dag.py] Traceback (most recent call last):
File "/usr/local/airflow/.local/lib/python3.8/site-packages/airflow/models/baseoperator.py", line 390, in apply_defaults
result = func(self, **kwargs, default_args=default_args)
File "/usr/local/airflow/.local/lib/python3.8/site-packages/airflow/models/baseoperator.py", line 743, in __init__
raise AirflowException(
airflow.exceptions.AirflowException: Invalid arguments were passed to EcsOperator (task_id: my-task). Invalid arguments were:
**kwargs: {'aws_conn_id': 'aws_default'}
```
is there a fix?
--
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] ferruzzi commented on a diff in pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r935896745
##########
airflow/contrib/operators/ecs_operator.py:
##########
@@ -1,30 +0,0 @@
-#
Review Comment:
When I revert this, what is the best way to handle it? Should I import all of the new ECS Operators fro this PR and add them to `__all__`, or only update the names of the ones which were already included in there?
--
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] ferruzzi commented on a diff in pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r938969923
##########
airflow/contrib/operators/ecs_operator.py:
##########
@@ -1,30 +0,0 @@
-#
Review Comment:
@eladkal - @potiuk mentioned wanting to get a provider package release out and Amazon would be getting a major release, I'm hoping to get this in on that but I'm not really sure what needs to be done here now. Any help would be appreciated.
--
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] ferruzzi commented on a diff in pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r938994761
##########
airflow/contrib/operators/ecs_operator.py:
##########
@@ -1,30 +0,0 @@
-#
Review Comment:
ACK. Will rebase and do a force-push. Thanks to both of you for the help.
--
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] ferruzzi commented on a diff in pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #25413:
URL: https://github.com/apache/airflow/pull/25413#discussion_r938995880
##########
airflow/contrib/operators/ecs_operator.py:
##########
@@ -1,30 +0,0 @@
-#
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.
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 pull request #25413: Refactor monolithic ECS Operator into Operators, Sensors, and a Hook
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25413:
URL: https://github.com/apache/airflow/pull/25413#issuecomment-1209695632
> If the tests pass, do you think we should be fine with releasing it in this upcoming wave of providers?
Ah yeah. I see the comment now :)
--
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