beobest2 commented on code in PR #38658:
URL: https://github.com/apache/airflow/pull/38658#discussion_r1604169292
##########
airflow/providers/amazon/aws/operators/emr.py:
##########
@@ -617,6 +617,14 @@ def execute(self, context: Context) -> str | None:
job_id=self.job_id,
aws_conn_id=self.aws_conn_id,
waiter_delay=self.poll_interval,
+ waiter_max_attempts=self.max_polling_attempts,
+ )
+ if self.max_polling_attempts
Review Comment:
@syedahsn
What you just mentioned was my initial approach.
However, after analyzing the code, I found that modifying only the input
type of
[EmrStepSensorTrigger](https://github.com/apache/airflow/blob/main/airflow/providers/amazon/aws/triggers/emr.py#L213)
results in a discrepancy with its parent class, violating the Liskov
Substitution Principle (LSP). If we were to modify the parent class,
[AwsBaseWaiterTrigger](https://github.com/apache/airflow/blob/main/airflow/providers/amazon/aws/triggers/base.py#L31),
we would also need to change the type of `waiter_max_attempts` in the
[async_await](https://github.com/apache/airflow/blob/main/airflow/providers/amazon/aws/utils/waiter_with_logging.py#L88)
utility that accepts this input. This would require handling the exception
when `waiter_max_attempts` is None.
(https://github.com/apache/airflow/blob/main/airflow/providers/amazon/aws/utils/waiter_with_logging.py#L119)
Additionally, this function is widely used across AWS operators, and we
cannot predict what other side effects it might cause.
Considering these factors, I believe the current code modification, while
not entirely clean, is the best approach.
--
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]
