potiuk commented on code in PR #29077:
URL: https://github.com/apache/airflow/pull/29077#discussion_r1084317558
##########
airflow/providers/amazon/aws/hooks/sagemaker.py:
##########
@@ -1154,19 +1154,35 @@ def stop_pipeline(
:return: Status of the pipeline execution after the operation.
One of 'Executing'|'Stopping'|'Stopped'|'Failed'|'Succeeded'.
"""
- try:
-
self.conn.stop_pipeline_execution(PipelineExecutionArn=pipeline_exec_arn)
- except ClientError as ce:
- # we have to rely on the message to catch the right error here,
because its type
- # (ValidationException) is shared with other kinds of error (for
instance, badly formatted ARN)
- if (
- not fail_if_not_running
- and "Only pipelines with 'Executing' status can be stopped" in
ce.response["Error"]["Message"]
- ):
- self.log.warning("Cannot stop pipeline execution, as it was
not running: %s", ce)
- else:
- self.log.error(ce)
- raise
+ retries = 2 # i.e. 3 calls max, 1 initial + 2 retries
Review Comment:
For those cases, I think we should only add configurable options if we
cannot figure our sensible defaults - we already have too many options accross
Airflow code.
Number of retries in this case is kind of reasonable (even if it is magic).
And I also think having a comment like that:
```
retries = 2 # i.e. 3 calls max, 1 initial + 2 retries
```
is FAR better than a constant defined somewhere at the top of the file. You
do not have to look it up in the file. There is absolutely no reason to create
a constant if it is used in one place and comment describing it. It does not
solve any problem, it only adds the need to make an extra lookup.
--
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
