Skip to content

Decrease default value of waiter_max_attempts in MwaaTriggerDagRunOperator - #49136

Merged
vincbeck merged 1 commit into
apache:mainfrom
aws-mwaa:vincbeck/bedrock_sys_test
Apr 11, 2025
Merged

vincbeck merged 1 commit into
apache:mainfrom
aws-mwaa:vincbeck/bedrock_sys_test

Conversation

@vincbeck

Copy link
Copy Markdown
Contributor

Having a timeout of 12 hours is a bit too much.


^ 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 airflow-core/newsfragments.

@boring-cyborg boring-cyborg Bot added area:providers provider:amazon AWS/Amazon - related issues labels Apr 11, 2025
@vincbeck
vincbeck merged commit a4a6e2b into apache:main Apr 11, 2025
@vincbeck
vincbeck deleted the vincbeck/bedrock_sys_test branch April 11, 2025 17:59

@o-nikolas o-nikolas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

12 hours is the max runtime for an MWAA task (as of today), so I can see where that number came from. Leaving it up to the user to decide if they want anything smaller seems reasonable to me, and to not be opinionated on our end what is a good time for them.

However, if everyone else prefers this shorter time, I'm happy to commit to that :)

@vincbeck

Copy link
Copy Markdown
Contributor Author

12 hours is the max runtime for an MWAA task (as of today), so I can see where that number came from. Leaving it up to the user to decide if they want anything smaller seems reasonable to me, and to not be opinionated on our end what is a good time for them.

However, if everyone else prefers this shorter time, I'm happy to commit to that :)

That's where the 12 hours was coming from :) Knowing this it makes sense, I do not think it is worth to update it back now it is merged though.

@ramitkataria

ramitkataria commented Apr 11, 2025 •

Copy link
Copy Markdown
Contributor

I just realized that the sensor will still wait for 12 hours:

Also, the docstrings contain default values so it looks like another PR will be needed. I can do it if you'd like :)

@vincbeck

Copy link
Copy Markdown
Contributor Author

I just realized that the sensor will still wait for 12 hours:

Also, the docstrings contain default values so it looks like another PR will be needed. I can do it if you'd like :)

Please do :) In that case, I'd follow Niko instructions and basically revert what I did but update the system test to only wait for, say, 15 mins?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:amazon AWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants