ferruzzi commented on code in PR #29761:
URL: https://github.com/apache/airflow/pull/29761#discussion_r1119241035
##########
airflow/providers/amazon/aws/waiters/ecs.json:
##########
@@ -0,0 +1,81 @@
+{
+ "version": 2,
+ "waiters": {
+ "cluster_active": {
+ "operation": "DescribeClusters",
+ "delay": 15,
+ "maxAttempts": 60,
+ "acceptors": [
+ {
+ "expected": "ACTIVE",
+ "matcher": "pathAny",
+ "state": "success",
+ "argument": "clusters[].status"
+ },
+ {
+ "expected": "FAILED",
+ "matcher": "pathAny",
+ "state": "failure",
+ "argument": "clusters[].status"
+ },
+ {
+ "expected": "INACTIVE",
+ "matcher": "pathAny",
+ "state": "failure",
+ "argument": "clusters[].status"
+ },
+ {
+ "expected": "MISSING",
+ "matcher": "pathAny",
+ "state": "failure",
+ "argument": "failures[].reason"
+ }
+ ]
+ },
+ "cluster_inactive": {
+ "operation": "DescribeClusters",
+ "delay": 15,
+ "maxAttempts": 60,
+ "acceptors": [
+ {
+ "expected": "INACTIVE",
+ "matcher": "pathAny",
+ "state": "success",
+ "argument": "clusters[].status"
+ },
+ {
+ "expected": "MISSING",
+ "matcher": "pathAny",
+ "state": "success",
+ "argument": "failures[].reason"
+ }
+ ]
+ },
+ "task_definition_active": {
+ "operation": "DescribeTaskDefinition",
+ "delay": 15,
+ "maxAttempts": 60,
+ "acceptors": [
+ {
+ "expected": "ACTIVE",
+ "matcher": "path",
+ "state": "success",
+ "argument": "taskDefinition.status"
+ }
+ ]
+ },
+ "task_definition_inactive": {
Review Comment:
Would `'DELETE_IN_PROGRESS'` be a success state here?
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -92,32 +96,45 @@ def __init__(
*,
cluster_name: str,
create_cluster_kwargs: dict | None = None,
- wait_for_completion: bool = True,
+ wait_for_completion: bool = False,
Review Comment:
Does this require a deprecation warning since it's changing default behavior?
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -39,7 +39,7 @@
EcsTaskDefinitionStates,
should_retry_eni,
)
-from airflow.providers.amazon.aws.sensors.ecs import EcsClusterStateSensor,
EcsTaskDefinitionStateSensor
+from airflow.utils.helpers import prune_dict
Review Comment:
Nice. I have a bunch of EMR waiters I have been workigb on converting over
but put on pause to verify how they behave if `None` is passed in, but this
would solve that nncely. I'll get the EMR ones up this week and tag you in
them. :+1:
##########
airflow/providers/amazon/aws/hooks/ecs.py:
##########
@@ -55,7 +55,7 @@ def should_retry_eni(exception: Exception):
return False
-class EcsClusterStates(Enum):
+class EcsClusterStates(str, Enum):
Review Comment:
Yeah, I was hoping to use StrEnum there but... not yet. :( I didn't know
you could do it this way, nice.
##########
airflow/providers/amazon/aws/waiters/ecs.json:
##########
@@ -0,0 +1,81 @@
+{
Review Comment:
Love seeing this put to use. I think it's so much cleaner in the
implementation. :+1:
##########
airflow/providers/amazon/aws/waiters/ecs.json:
##########
@@ -0,0 +1,81 @@
+{
+ "version": 2,
+ "waiters": {
+ "cluster_active": {
+ "operation": "DescribeClusters",
+ "delay": 15,
+ "maxAttempts": 60,
+ "acceptors": [
+ {
+ "expected": "ACTIVE",
+ "matcher": "pathAny",
+ "state": "success",
+ "argument": "clusters[].status"
+ },
+ {
+ "expected": "FAILED",
+ "matcher": "pathAny",
+ "state": "failure",
+ "argument": "clusters[].status"
+ },
+ {
+ "expected": "INACTIVE",
+ "matcher": "pathAny",
+ "state": "failure",
+ "argument": "clusters[].status"
+ },
+ {
+ "expected": "MISSING",
+ "matcher": "pathAny",
+ "state": "failure",
+ "argument": "failures[].reason"
Review Comment:
Interesting catch. :+1:
##########
airflow/providers/amazon/aws/waiters/ecs.json:
##########
@@ -0,0 +1,81 @@
+{
+ "version": 2,
+ "waiters": {
+ "cluster_active": {
+ "operation": "DescribeClusters",
+ "delay": 15,
+ "maxAttempts": 60,
+ "acceptors": [
+ {
+ "expected": "ACTIVE",
+ "matcher": "pathAny",
+ "state": "success",
+ "argument": "clusters[].status"
+ },
+ {
+ "expected": "FAILED",
+ "matcher": "pathAny",
+ "state": "failure",
+ "argument": "clusters[].status"
+ },
+ {
+ "expected": "INACTIVE",
+ "matcher": "pathAny",
+ "state": "failure",
+ "argument": "clusters[].status"
+ },
+ {
+ "expected": "MISSING",
+ "matcher": "pathAny",
+ "state": "failure",
+ "argument": "failures[].reason"
+ }
+ ]
+ },
+ "cluster_inactive": {
+ "operation": "DescribeClusters",
+ "delay": 15,
+ "maxAttempts": 60,
+ "acceptors": [
+ {
+ "expected": "INACTIVE",
+ "matcher": "pathAny",
+ "state": "success",
+ "argument": "clusters[].status"
+ },
+ {
+ "expected": "MISSING",
+ "matcher": "pathAny",
+ "state": "success",
+ "argument": "failures[].reason"
+ }
+ ]
+ },
+ "task_definition_active": {
Review Comment:
No failure states for this one? I think if the status goes to
`'DELETE_IN_PROGRESS'` it can fail early. Not sure where `'INACTIVE'` is on
the state flow on this call 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.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
