You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ferruzzi (via GitHub)" <gi...@apache.org> on 2023/02/27 19:56:10 UTC
[GitHub] [airflow] ferruzzi commented on a diff in pull request #29761: Use waiters in ECS Operators instead of inner sensors
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org