You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "vincbeck (via GitHub)" <gi...@apache.org> on 2023/06/20 21:50:33 UTC
[GitHub] [airflow] vincbeck commented on a diff in pull request #32036: Add a deferrable mode to `BatchCreateComputeEnvironmentOperator`
vincbeck commented on code in PR #32036:
URL: https://github.com/apache/airflow/pull/32036#discussion_r1235924782
##########
airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -428,31 +431,40 @@ def __init__(
unmanaged_v_cpus: int | None = None,
service_role: str | None = None,
tags: dict | None = None,
- max_retries: int | None = None,
+ poll_interval: int = 30,
+ max_retries: int = 120,
Review Comment:
This is breaking change. If a user passes None today, it will break at next release
##########
airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -428,31 +431,40 @@ def __init__(
unmanaged_v_cpus: int | None = None,
service_role: str | None = None,
tags: dict | None = None,
- max_retries: int | None = None,
+ poll_interval: int = 30,
+ max_retries: int = 120,
status_retries: int | None = None,
aws_conn_id: str | None = None,
region_name: str | None = None,
+ deferrable: bool = True,
**kwargs,
):
super().__init__(**kwargs)
+ if status_retries is not None:
+ warnings.warn(
+ "The `status_retries` parameter is unused and should be removed. "
+ "It'll be deleted in a future version.",
+ AirflowProviderDeprecationWarning,
+ stacklevel=2,
+ )
+
self.compute_environment_name = compute_environment_name
self.environment_type = environment_type
self.state = state
self.unmanaged_v_cpus = unmanaged_v_cpus
self.compute_resources = compute_resources
self.service_role = service_role
self.tags = tags or {}
+ self.poll_interval = poll_interval
self.max_retries = max_retries
- self.status_retries = status_retries
self.aws_conn_id = aws_conn_id
self.region_name = region_name
+ self.deferrable = deferrable
@cached_property
def hook(self):
"""Create and return a BatchClientHook."""
return BatchClientHook(
- max_retries=self.max_retries,
- status_retries=self.status_retries,
Review Comment:
Is not it a breaking change as well here?
##########
airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -428,31 +431,40 @@ def __init__(
unmanaged_v_cpus: int | None = None,
service_role: str | None = None,
tags: dict | None = None,
- max_retries: int | None = None,
+ poll_interval: int = 30,
+ max_retries: int = 120,
status_retries: int | None = None,
aws_conn_id: str | None = None,
region_name: str | None = None,
+ deferrable: bool = True,
**kwargs,
):
super().__init__(**kwargs)
+ if status_retries is not None:
Review Comment:
Should we remove it from the list of parameters then? To not encourage people using it
##########
airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -428,31 +431,40 @@ def __init__(
unmanaged_v_cpus: int | None = None,
service_role: str | None = None,
tags: dict | None = None,
- max_retries: int | None = None,
+ poll_interval: int = 30,
Review Comment:
Please update docstring
##########
airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -428,31 +431,40 @@ def __init__(
unmanaged_v_cpus: int | None = None,
service_role: str | None = None,
tags: dict | None = None,
- max_retries: int | None = None,
+ poll_interval: int = 30,
+ max_retries: int = 120,
status_retries: int | None = None,
aws_conn_id: str | None = None,
region_name: str | None = None,
+ deferrable: bool = True,
Review Comment:
`True` by default? I would not do that. That's a breaking change and I am not sure we want to enable the deferrable mode by default
##########
airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -468,6 +480,21 @@ def execute(self, context: Context):
"serviceRole": self.service_role,
"tags": self.tags,
}
- self.hook.client.create_compute_environment(**trim_none_values(kwargs))
+ response = self.hook.client.create_compute_environment(**trim_none_values(kwargs))
+ arn = response["computeEnvironmentArn"]
Review Comment:
If you move this one inside the if statement
--
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