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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
