Skip to content

Fix exponential backoff float overflow#48057

Closed
alealandreev wants to merge 5 commits into
apache:v2-10-testfrom
alealandreev:patch-2
Closed

Fix exponential backoff float overflow#48057
alealandreev wants to merge 5 commits into
apache:v2-10-testfrom
alealandreev:patch-2

Conversation

@alealandreev

@alealandreev alealandreev commented Mar 21, 2025

Copy link
Copy Markdown

closes: #47971


^ 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.

@ashb

ashb commented Mar 23, 2025

Copy link
Copy Markdown
Member

This is a big enough change it probably needs some tests.

However rather than maintaining this logic ourselves is it possible make use of https://github.com/hynek/stamina or https://tenacity.readthedocs.io/en/latest/ instead please?

@eladkal

eladkal commented Mar 24, 2025

Copy link
Copy Markdown
Contributor

This PR targets v2-10-test branch. Is the issue affect only 2.10? The bug does not exist in main branch?

@potiuk

potiuk commented Mar 30, 2025

Copy link
Copy Markdown
Member

Yeah. I rebased it. And this was my suggestion to just limit it - there was a much more complicated version of this PR #48051 - > I think for Airflow 2 that one should be "good enough".

@potiuk

potiuk commented Mar 30, 2025

Copy link
Copy Markdown
Member

However we have to get the 2-10-test branch back in order which I believe we have @jason810496 working on it in #48404 and I am going to turn my attention to it as well finally to help.

@potiuk

potiuk commented Mar 30, 2025

Copy link
Copy Markdown
Member

However rather than maintaining this logic ourselves is it possible make use of https://github.com/hynek/stamina or https://tenacity.readthedocs.io/en/latest/ instead please?

BTW. @ashb -> It would be interesitng to see if one of those handles it better, but that might be difficult to "test" it (and I would hate to add a dependency for it. We do not have stamina so far, but we do have tenacity, so if tenacity can be used to handle it - cool, but other than that, adding other dependency here feels like leftpad case for the 3 lines of code.

@perry2of5

Copy link
Copy Markdown
Contributor

I looked at using tenacity bigger rewrite than is perhaps warranted. Also, I added the missing tests @ashb wanted in a PR against the branch.

@alealandreev alealandreev closed this by deleting the head repository Apr 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants