You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Taragolis (via GitHub)" <gi...@apache.org> on 2023/03/01 11:16:56 UTC
[GitHub] [airflow] Taragolis commented on a diff in pull request #29822: Implement custom boto waiters for some EMR operators
Taragolis commented on code in PR #29822:
URL: https://github.com/apache/airflow/pull/29822#discussion_r1121525813
##########
airflow/providers/amazon/aws/operators/emr.py:
##########
@@ -191,8 +196,23 @@ def __init__(
aws_conn_id: str = "aws_default",
waiter_countdown: int = 25 * 60,
waiter_check_interval_seconds: int = 60,
+ # TODO: waiter_max_attempts and waiter_delay should default to None when the other two are deprecated.
+ waiter_max_attempts: int | ArgNotSet = NOTSET,
+ waiter_delay: int | ArgNotSet = NOTSET,
**kwargs: Any,
):
+ if waiter_max_attempts is NOTSET:
+ warnings.warn(
+ "The parameter waiter_countdown has been deprecated to standardize "
+ "naming conventions. Please use waiter_max_attempts instead. In the "
+ "future this will default to None and defer to the waiter's default value."
+ )
Review Comment:
```suggestion
if waiter_max_attempts is NOTSET:
warnings.warn(
"The parameter waiter_countdown has been deprecated to standardize "
"naming conventions. Please use waiter_max_attempts instead. In the "
"future this will default to None and defer to the waiter's default value."
)
waiter_max_attempts = waiter_countdown // waiter_check_interval_seconds
```
What if we change value here so we could change this
```python
self.waiter_max_attempts = (
(waiter_countdown // waiter_check_interval_seconds)
if waiter_max_attempts is NOTSET
else waiter_max_attempts
)
```
to
```python
self.waiter_max_attempts = waiter_max_attempts
```
##########
tests/providers/amazon/aws/utils/test_waiter.py:
##########
@@ -38,6 +38,17 @@ def generate_response(state: str) -> dict[str, Any]:
}
+def assert_expected_waiter_type(waiter: mock.MagicMock, expected: str):
+ """
+ There does not appear to be a straight-forward way to assert the type of waiter.
+ Instead, get the class name and check if it contains the expected name.
+
+ :param waiter: A mocked Boto3 Waiter object.
+ :param expected: The expected class name of the Waiter object, for example "ClusterActive".
+ """
+ assert expected in str(type(waiter.call_args[0][0]))
Review Comment:
Cross-PR suggestion
I've checked that specific hook has access to waiters, in my case only custom waiters
https://github.com/apache/airflow/blob/cc8ce585cad5849a561404cf3d2cb726c207a2cb/tests/providers/amazon/aws/waiters/test_custom_waiters.py#L127-L132
##########
airflow/providers/amazon/aws/operators/emr.py:
##########
@@ -159,9 +160,11 @@ class EmrStartNotebookExecutionOperator(BaseOperator):
group to associate with the master instance of the EMR cluster for this notebook execution.
:param tags: Optional list of key value pair to associate with the notebook execution.
:param waiter_countdown: Total amount of time the operator will wait for the notebook to stop.
- Defaults to 25 * 60 seconds.
+ Defaults to 25 * 60 seconds. (Deprecated. Please use waiter_max_attempts.)
:param waiter_check_interval_seconds: Number of seconds between polling the state of the notebook.
- Defaults to 60 seconds.
Review Comment:
Not strong opinion, but what if we move deprecated values to the bottom of docstring and `__init__` signature.
It should not affect anything because Operators only use keyword arguments however less important arguments would be in the bottom
##########
airflow/providers/amazon/aws/operators/emr.py:
##########
@@ -191,8 +196,23 @@ def __init__(
aws_conn_id: str = "aws_default",
waiter_countdown: int = 25 * 60,
waiter_check_interval_seconds: int = 60,
+ # TODO: waiter_max_attempts and waiter_delay should default to None when the other two are deprecated.
+ waiter_max_attempts: int | ArgNotSet = NOTSET,
+ waiter_delay: int | ArgNotSet = NOTSET,
Review Comment:
```suggestion
waiter_max_attempts: int | None |ArgNotSet = NOTSET,
waiter_delay: int | None | ArgNotSet = NOTSET,
```
I guess None it is already valid value for this arguments
##########
airflow/providers/amazon/aws/operators/emr.py:
##########
@@ -191,8 +196,23 @@ def __init__(
aws_conn_id: str = "aws_default",
waiter_countdown: int = 25 * 60,
waiter_check_interval_seconds: int = 60,
+ # TODO: waiter_max_attempts and waiter_delay should default to None when the other two are deprecated.
+ waiter_max_attempts: int | ArgNotSet = NOTSET,
+ waiter_delay: int | ArgNotSet = NOTSET,
**kwargs: Any,
):
+ if waiter_max_attempts is NOTSET:
+ warnings.warn(
+ "The parameter waiter_countdown has been deprecated to standardize "
+ "naming conventions. Please use waiter_max_attempts instead. In the "
+ "future this will default to None and defer to the waiter's default value."
+ )
Review Comment:
And the same to waiter_delay
--
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