0x26res commented on code in PR #39608:
URL: https://github.com/apache/airflow/pull/39608#discussion_r1600028210
##########
airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -206,8 +206,8 @@ def __init__(
self.scheduling_priority_override = scheduling_priority_override
self.array_properties = array_properties
self.parameters = parameters or {}
- self.retry_strategy = retry_strategy or {}
- if not self.retry_strategy.get("attempts", None):
+ self.retry_strategy = retry_strategy
+ if self.retry_strategy is not None and not
self.retry_strategy.get("attempts", None):
Review Comment:
Thanks for the suggestion.
I think in the case where `self.retry_strategy = {}`, the intention of the
existing code is to update it to `{"attempts": 1}`. But `bool({})` evaluate to
False. So unless I'm mistaken, I don't think your suggestion would
work/preserve the existing behavior.
Having that said, that bit of code was introduced because retry_strategy was
defaulting to `{}`, causing issue in aws batch. Given now it default to None,
we don't have that issue any more (AWS batch can handle None, but can't handle
`{}` for retry strategy). So I could just get rid of this. If the user wants to
specify a retry_strategy, it's their responsibility to set the `attempts`
correctly. We shouldn't be mutating the user input in place anyway.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
