-
Notifications
You must be signed in to change notification settings - Fork 14.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support timeout
in BatchOperator
#45619
Support timeout
in BatchOperator
#45619
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
@@ -208,6 +210,7 @@ def __init__( | |||
self.poll_interval = poll_interval | |||
self.awslogs_enabled = awslogs_enabled | |||
self.awslogs_fetch_interval = awslogs_fetch_interval | |||
self.timeout = timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be mistakenly confused with Airflow task timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would batch_job_timeout
be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think keeping same/similar name as boto3 interface is simpler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please lets not use timeout
.
You can use the same name as boto3 or choose something else but not names that can cause confusion with BaseOperator parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boto3_timeout
would make sense I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to boto3_timeout
, batch_job_timeout
, job_timeout
etc, whatever is acceptable.
@@ -70,6 +70,7 @@ def setup_method(self, _, get_client_type_mock): | |||
aws_conn_id="airflow_test", | |||
region_name="eu-west-1", | |||
tags={}, | |||
timeout={"attemptDurationSeconds": 3600}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vincbeck Should I be adding timeout={"attemptDurationSeconds": 3600},
to assert_called_once_with
calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please
Something is up, I'll try to get the tests working locally |
Moved to #45660 to avoid merge commits |
When using a deferrable BatchOperator, it is possible to specify the
max_retries
andpoll_interval
, but this does not appear to terminate the job once the task times out.Having the ability to specify the timeout and letting the job be terminated will allow us to enforce strict timeouts on critical pipelines running on Fargate via AWS Batch.
This PR adds support to allow specifying
timeout
in the BatchOperator constructor:https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/batch/client/submit_job.html
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.