-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Add waiter config params to emr.add_job_flow_steps #28464
Add waiter config params to emr.add_job_flow_steps #28464
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 Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
@eladkal could you please take a look also? I didn't intend to remove you from the reviewers but it happened |
@Taragolis could you please merge this one? |
Just to make sure to set you expectations. Please do not ping people like that. It's quite selfish. Be patient. There are 100s of PR being possibly looked at in parallel and merged in due time. You are looking at 1 PR while people who merge them at many @ShelRoman. be mindful for people time and accept the fact that people here work in their free time - and do not expect "immediate" reactions. Watch my talk https://www.youtube.com/watch?v=G6VjYvKr2wQ&list=PLGudixcDaxY2LxjeHpZRtzq7miykjjFOn&index=55 about empathy to understand why. |
@potiuk thanks for the clarification and the link. :) |
Awesome work, congrats on your first merged pull request! |
A small tweak related to #27286
The hardcoded config isn't usable with jobs that are running longer than ~500 seconds.
Moved those values into default arguments of the
add_job_flow_steps()
function which won't change default behaviour but will add some flexibility for use cases with longer emr steps.Also considered option to pass whole config dict instead of
waiter_delay
andwaiter_max_attempts
, but these changes are smaller.