syedahsn commented on code in PR #29077:
URL: https://github.com/apache/airflow/pull/29077#discussion_r1083228639
##########
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:
Why not make this a configurable parameter? I understand that this code
change is to catch an unlikely transient error, but I think adding "magic"
numbers reduces code readability. If not a configurable parameter, maybe add a
constant at the top with a clear name of what the number is for.
--
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]
