Skip to content

fixed a small float value of retry_backoff#8295

Merged
auvipy merged 5 commits intocelery:mainfrom
ipakeev:small_float_retry_backoff
Jun 18, 2023
Merged

fixed a small float value of retry_backoff#8295
auvipy merged 5 commits intocelery:mainfrom
ipakeev:small_float_retry_backoff

Conversation

@ipakeev
Copy link
Copy Markdown
Contributor

@ipakeev ipakeev commented Jun 3, 2023

Note: Before submitting this pull request, please review our contributing
guidelines
.

Description

I encountered an unexpected behavior: if I set retry_backoff to 0.0 < {value} < 1.0, all retries will run after default_retry_delay (180 seconds).
I don't think it makes much sense to allow a floating retry_backoff value, so I propose just rounding the {value} up to 1.

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 3, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (f51f805) 87.16% compared to head (340bf21) 87.15%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8295      +/-   ##
==========================================
- Coverage   87.16%   87.15%   -0.01%     
==========================================
  Files         148      148              
  Lines       18469    18469              
  Branches     2524     3148     +624     
==========================================
- Hits        16098    16097       -1     
- Misses       2092     2094       +2     
+ Partials      279      278       -1     
Flag Coverage Δ
unittests 87.12% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
celery/app/autoretry.py 93.54% <100.00%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@auvipy auvipy self-requested a review June 4, 2023 05:17
Copy link
Copy Markdown
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

can you please add additional test cases for this change?

@auvipy auvipy added this to the 5.3.x milestone Jun 10, 2023
@ipakeev
Copy link
Copy Markdown
Contributor Author

ipakeev commented Jun 11, 2023

@auvipy Added tests.

@auvipy
Copy link
Copy Markdown
Member

auvipy commented Jun 12, 2023

it would be great to add additional integration tests

@ipakeev
Copy link
Copy Markdown
Contributor Author

ipakeev commented Jun 13, 2023

Added additional tests.

@auvipy what kind of integration tests do you expect? I've not found any integration cases in which retry_backoff is used.

@auvipy auvipy requested review from a team and auvipy June 14, 2023 06:25
retry_kwargs['countdown'] = \
get_exponential_backoff_interval(
factor=retry_backoff,
factor=int(max(1.0, retry_backoff)),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

are we going to turn everything into 1 only or multiple of 1.0?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

All values less then 1 turn into 1 (except zero). For the other values, the behavior remains the same. I'm using 1.0 (not 1) just for linter.

@auvipy auvipy merged commit fc1c38a into celery:main Jun 18, 2023
@auvipy
Copy link
Copy Markdown
Member

auvipy commented Jun 18, 2023

merging with the hope that no regression was introduced! if we got any report, please try to collaborate if needed. thanks for your contribution.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants