You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "vandonr-amz (via GitHub)" <gi...@apache.org> on 2023/06/14 00:15:25 UTC
[GitHub] [airflow] vandonr-amz opened a new pull request, #31884: remove 'wait_for_completion' from ecs register/deregister operators
vandonr-amz opened a new pull request, #31884:
URL: https://github.com/apache/airflow/pull/31884
those operations are actually completed instantly.
the doc to deregister says it explicitely:
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/deregister-task-definition.html
> When you deregister a task definition revision, it’s immediately marked as INACTIVE.
to register a task, it's less clear, but task definition states show no transition states https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-definition-state.html and expriments show that the task is always returned as active right after having been created.
--
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] vandonr-amz commented on a diff in pull request #31884: remove 'wait_for_completion' from ecs register/deregister operators
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #31884:
URL: https://github.com/apache/airflow/pull/31884#discussion_r1230228806
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -202,53 +202,39 @@ class EcsDeregisterTaskDefinitionOperator(EcsBaseOperator):
:param task_definition: The family and revision (family:revision) or full Amazon Resource Name (ARN)
of the task definition to deregister. If you use a family name, you must specify a revision.
- :param wait_for_completion: If True, waits for creation of the cluster to complete. (default: True)
- :param waiter_delay: The amount of time in seconds to wait between attempts,
- if not set then the default waiter value will be used.
- :param waiter_max_attempts: The maximum number of attempts to be made,
- if not set then the default waiter value will be used.
"""
- template_fields: Sequence[str] = ("task_definition", "wait_for_completion")
+ template_fields: Sequence[str] = "task_definition"
Review Comment:
is it going to break ? 🤔
My thinking was just that the "right" value wouldn't be resolved, but that ain't a problem if we don't use that value.
--
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] utkarsharma2 commented on a diff in pull request #31884: remove 'wait_for_completion' from ecs register/deregister operators
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31884:
URL: https://github.com/apache/airflow/pull/31884#discussion_r1229125242
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -215,40 +212,22 @@ def __init__(
self,
*,
task_definition: str,
- wait_for_completion: bool = True,
- waiter_delay: int | None = None,
- waiter_max_attempts: int | None = None,
+ wait_for_completion: bool = True, # obsolete
Review Comment:
Should we also add a warning saying `wait_for_completion/waiter_delay/waiter_max_attempts` has no effect going forward and point to the documentation in AWS? And also, a deprecation warning assuming they will eventually be removed would be nice. 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] phanikumv commented on a diff in pull request #31884: remove 'wait_for_completion' from ecs register/deregister operators
Posted by "phanikumv (via GitHub)" <gi...@apache.org>.
phanikumv commented on code in PR #31884:
URL: https://github.com/apache/airflow/pull/31884#discussion_r1229183867
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -215,40 +212,22 @@ def __init__(
self,
*,
task_definition: str,
- wait_for_completion: bool = True,
- waiter_delay: int | None = None,
- waiter_max_attempts: int | None = None,
+ wait_for_completion: bool = True, # obsolete
Review Comment:
+1 , we need to add a deprecation warning for users who already use these params so that their DAG wont break. Other than that everything else looks good
--
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 #31884: remove 'wait_for_completion' from ecs register/deregister operators
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #31884:
URL: https://github.com/apache/airflow/pull/31884#discussion_r1230215453
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -202,53 +202,39 @@ class EcsDeregisterTaskDefinitionOperator(EcsBaseOperator):
:param task_definition: The family and revision (family:revision) or full Amazon Resource Name (ARN)
of the task definition to deregister. If you use a family name, you must specify a revision.
- :param wait_for_completion: If True, waits for creation of the cluster to complete. (default: True)
- :param waiter_delay: The amount of time in seconds to wait between attempts,
- if not set then the default waiter value will be used.
- :param waiter_max_attempts: The maximum number of attempts to be made,
- if not set then the default waiter value will be used.
"""
- template_fields: Sequence[str] = ("task_definition", "wait_for_completion")
+ template_fields: Sequence[str] = "task_definition"
Review Comment:
This is a breaking change
Even if it doesn't mean anything if user has templating over this field and we remove it his code will break.
We should keep it until we remove the parameter
--
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 #31884: Deprecate `wait_for_completion` from `EcsRegisterTaskDefinitionOperator` and `EcsDeregisterTaskDefinitionOperator`
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal merged PR #31884:
URL: https://github.com/apache/airflow/pull/31884
--
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] vandonr-amz commented on a diff in pull request #31884: remove 'wait_for_completion' from ecs register/deregister operators
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #31884:
URL: https://github.com/apache/airflow/pull/31884#discussion_r1230229340
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -202,53 +202,39 @@ class EcsDeregisterTaskDefinitionOperator(EcsBaseOperator):
:param task_definition: The family and revision (family:revision) or full Amazon Resource Name (ARN)
of the task definition to deregister. If you use a family name, you must specify a revision.
- :param wait_for_completion: If True, waits for creation of the cluster to complete. (default: True)
- :param waiter_delay: The amount of time in seconds to wait between attempts,
- if not set then the default waiter value will be used.
- :param waiter_max_attempts: The maximum number of attempts to be made,
- if not set then the default waiter value will be used.
"""
- template_fields: Sequence[str] = ("task_definition", "wait_for_completion")
+ template_fields: Sequence[str] = "task_definition"
Review Comment:
like, jinja templates will still be strings with "{{ whatever }}" and xcom values will be some weird types, but nothing breaking I'd say
--
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] vandonr-amz commented on a diff in pull request #31884: remove 'wait_for_completion' from ecs register/deregister operators
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #31884:
URL: https://github.com/apache/airflow/pull/31884#discussion_r1230149316
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -215,40 +212,22 @@ def __init__(
self,
*,
task_definition: str,
- wait_for_completion: bool = True,
- waiter_delay: int | None = None,
- waiter_max_attempts: int | None = None,
+ wait_for_completion: bool = True, # obsolete
Review Comment:
hmm yeah right otherwise users won't remove them
--
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 #31884: remove 'wait_for_completion' from ecs register/deregister operators
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #31884:
URL: https://github.com/apache/airflow/pull/31884#discussion_r1232857696
##########
airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -202,53 +202,39 @@ class EcsDeregisterTaskDefinitionOperator(EcsBaseOperator):
:param task_definition: The family and revision (family:revision) or full Amazon Resource Name (ARN)
of the task definition to deregister. If you use a family name, you must specify a revision.
- :param wait_for_completion: If True, waits for creation of the cluster to complete. (default: True)
- :param waiter_delay: The amount of time in seconds to wait between attempts,
- if not set then the default waiter value will be used.
- :param waiter_max_attempts: The maximum number of attempts to be made,
- if not set then the default waiter value will be used.
"""
- template_fields: Sequence[str] = ("task_definition", "wait_for_completion")
+ template_fields: Sequence[str] = "task_definition"
Review Comment:
You are right. resolving
--
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