-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
Fix automatic termination issue in EmrOperator by ensuring waiter_max_attempts
is set for deferrable triggers
#38658
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)
|
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.
👍
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.
Good.
4e994ab
to
d45f471
Compare
can you add unit tests to cover this change? |
@eladkal I have also added unit tests. Please review |
Tests need to pass after your rebase from main. Otherwise looks good to merge |
67c7eda
to
bee2bb9
Compare
Running workflows again after recent commits |
@o-nikolas Previously, I was unable to properly test in my development environment because different dependency errors occurred. After solving the problem and running tests, I found that when deferrable is set to True, the |
CC @syedahsn |
bee2bb9
to
12e5369
Compare
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.
Left a couple of comments. Thanks for looking into this!
9d48300
to
b992536
Compare
b992536
to
4fb4fbe
Compare
4fb4fbe
to
23fad98
Compare
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.
LGTM
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
…max_attempts` is set for deferrable triggers (apache#38658)
…max_attempts` is set for deferrable triggers (apache#38658)
When running EMR jobs that require a long duration, setting the
waiter_max_attempts
in EmrOperator to a high value does not prevent the "waiter error: max attempts" error from occurring after a certain period.An issue has been identified with the AWS provider code.
The
EmrOperator
that calls this trigger does not allow users to inputwaiter_max_attempts
, resulting in the application of the default value of 600.airflow/providers/amazon/aws/triggers/emr.py
In other words, if the
poll_interval
is set to 60 seconds, the job will terminate after 10 hours (1 minute * 600) regardless of thewaiter_max_attempts
value specified by the user.To address this, modifications were made in
EmrOperator
to ensure that parameters provided by the user are also passed to theEmrContainerTrigger
.